Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implementation of WFJT limit & SCM_branch prompting #4369

Merged
merged 10 commits into from
Sep 16, 2019
Merged

Implementation of WFJT limit & SCM_branch prompting #4369

merged 10 commits into from
Sep 16, 2019

Conversation

AlanCoding
Copy link
Member

SUMMARY

Connect #1731

Does not yet do #1845

There will also be a need to apply this to #282

I believe that 282 will be in scope near-term, but 1845 will not.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME
  • API
AWX VERSION
6.0.0
ADDITIONAL INFORMATION

Heads up to @jbradberry, this is about as simple as this material is going to get, and that's not very simple. This does the abuse of Django character fields to make them nullable, and tries to make it so that scm_revision will follow suit in a relatively consistent pattern.

@jakemcdermott
Copy link
Contributor

recheck

@AlanCoding AlanCoding force-pushed the workflow_limit branch 2 times, most recently from 7bd8784 to e51c410 Compare August 15, 2019 14:12
@AlanCoding AlanCoding changed the title [WIP] Implementation of WFJT limit & prompting Implementation of WFJT limit & prompting Aug 16, 2019
@AlanCoding AlanCoding changed the title Implementation of WFJT limit & prompting Implementation of WFJT limit & SCM_branch prompting Aug 16, 2019
@AlanCoding
Copy link
Member Author

This implementation is still a half-way move toward making WFJTs full "config" objects. Basically, we have the opportunity to remove some of the really back hacks that had to be implemented for sliced jobs. See:

awx/awx/main/models/workflow.py

Lines 232 to 240 in 76a1099

try:
# config saved on the workflow job itself
wj_config = self.workflow_job.launch_config
except ObjectDoesNotExist:
wj_config = None
if wj_config:
accepted_fields, ignored_fields, errors = ujt_obj._accept_or_ignore_job_kwargs(**wj_config.prompts_dict())
accepted_fields.pop('extra_vars', None) # merge handled with other extra_vars later
data.update(accepted_fields)

I'm resisting doing a much broader refactor to get rid of this. Ideally this would use self.workflow_job.prompts_dict() instead of the config object. But it still wouldn't be clean, because of the exception for variable acceptance with extra_vars.

mabashian
mabashian previously requested changes Aug 21, 2019
View reviewed changes
Copy link
Member

@mabashian mabashian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great - I did find one scenario that's not covered in the UI currently. If you create a workflow and prompt for limit on launch (make sure this is the only promptable thing on the workflow) and launch that workflow, the prompt modal isn't shown and the job is simply launched. In order to fix this I think we need to change https://github.com/ansible/awx/blob/devel/awx/ui/client/lib/models/WorkflowJobTemplate.js#L57 to

    return (
        launchData.can_start_without_user_input &&
        !launchData.ask_inventory_on_launch &&
        !launchData.ask_variables_on_launch &&
        !launchData.survey_enabled &&
        !launchData.ask_scm_branch_on_launch &&
        !launchData.ask_limit_on_launch &&
        launchData.variables_needed_to_start.length === 0
    );

Note the addition of !launchData.ask_limit_on_launch && to that conditional. Otherwise I think this is looking good. I tested out the workflow node scenarios and ran into #4293 but that should be addressed by #4525

@AlanCoding
Copy link
Member Author

Thanks, good catch!

I was easily able to replicate this situation manually and confirmed that the latest commit fixes it.

While doing this, I noticed another odd thing - I couldn't set it to prompt for limit while creating the WFJT. That turned out to be because it was missing in yet another place. Also confirmed fixed in last commit.

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

dsesami
dsesami approved these changes Sep 10, 2019
View reviewed changes
Copy link
Contributor

@dsesami dsesami left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking really solid. Pinging @squidboylan on a final call for this on QE side, looks good from UI end.

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@softwarefactory-project-zuul
Copy link
Contributor

Build failed.

@AlanCoding
Copy link
Member Author

recheck

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

AlanCoding and others added 10 commits September 16, 2019 14:51
@AlanCoding
Implement WFJT prompting for limit & scm_branch
e8581f6 
add feature to UI and awxkit

restructure some details of create_unified_job
  for workflows to allow use of char_prompts
  hidden field
avoid conflict with sliced jobs in char_prompts copy logic

update developer docs

update migration reference

bump migration
@AlanCoding
Fix missing places for ask_limit and ask_scm_branch
1406ea3
@AlanCoding
adjust UI unit tests again
291528d 
bump migration

bump migration again
@AlanCoding
Consistently give WJ extra_vars as text
711c240
@AlanCoding
Deal with limit prompting in factory
01bb32e
@jakemcdermott @AlanCoding
add more details to workflow limit help text
8ac8fb9
@jakemcdermott @AlanCoding
psuedo -> pseudo
84a8559
@jakemcdermott @AlanCoding
comment fixup
9697e1b
@AlanCoding
bump migration
fdf9dd7
@AlanCoding
bump migration
e3c1189
@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded (gate pipeline).

@softwarefactory-project-zuul softwarefactory-project-zuul bot merged commit b5fa160 into ansible:devel Sep 16, 2019
@ryanpetrello ryanpetrello mentioned this pull request Sep 30, 2019
Open
@AlanCoding AlanCoding deleted the workflow_limit branch October 9, 2019 14:46
@dr1s dr1s mentioned this pull request Nov 7, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dsesami dsesami dsesami approved these changes

@squidboylan squidboylan squidboylan approved these changes

@mabashian mabashian mabashian approved these changes

@jakemcdermott jakemcdermott jakemcdermott approved these changes

@jlmitch5 jlmitch5 jlmitch5 approved these changes

@jbradberry jbradberry jbradberry approved these changes

Assignees

@jakemcdermott jakemcdermott

Labels
component:api component:ui type:enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@AlanCoding @jakemcdermott @dsesami @jbradberry @jlmitch5 @mabashian @squidboylan @awxbot