Allow JT specification and prompting for project branch #4265

AlanCoding · 2019-07-03T20:50:25Z

SUMMARY

Implements the core of #282

Decisions that are non-obvious from the prior discussion:

using the default branch with allow_override being True will behave the same as other projects (offline runs are possible). I still would argue that this isn't consistent, and will be a surprise to the user, but any decision here would have problems with communication.
prompting in workflows is allowed in the node prompts, same with schedules
playbook list is produced from project's default branch (for use as suggestions)

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

API

ADDITIONAL INFORMATION

We hope to handle in another PR:

organization as field on JT
cloning from a PR ref (may be done in Ansible core)

softwarefactory-project-zuul · 2019-07-03T20:50:33Z

Merge Failed.

This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset.

matburt · 2019-07-08T18:34:19Z

Can you explain this more?

using the default branch with allow_override being True will behave the same as other projects (offline runs are possible). I still would argue that this isn't consistent, and will be a surprise to the user, but any decision here would have problems with communication.

AlanCoding · 2019-07-08T18:54:13Z

Using example:

https://github.com/ansible/ansible/branches

Steps:

I create a project from the Ansible core repo with
- allow_override is True
- default scm_branch is devel
Right after creating it
- I run a job without overriding the branch
- I run a job that runs with stable-2.8 branch
I return a week later and relaunch both of those jobs
- devel does not pull a newer revision
- stable-2.8 does pull the latest revision from github and runs that

This is because we run against the revision of the last "check"-type project update, which has its revision stored. Prompted, non-default branches, do not have their revision saved.

We can change that, but a lot of question need to be answered. This behavior both was the least invasive, and closest to the requirements discussed in planning. I wanted to give pushback on this, but I'm happy with it now. While it's surprising that the default branch behaves differently from the non-default branch, it would also be surprising that different values of allow_override would behave differently. So unless we save the revision of all branches (technically problematic in some cases, impossible in others), the current implementation seems like the best option.

I might take some of this and put it in the docs folder, that had slipped my mind so-far.

matburt

There's a lot going on in tasks.py here... I might want you to annotate what's going on and what you are working towards here, especially since it's going to affect project syncing overall.

matburt · 2019-07-08T18:37:34Z

awx/api/serializers.py

+        if 'allow_override' in attrs and self.instance:
+            if attrs['allow_override'] != self.instance:
+                raise serializers.ValidationError({
+                    'allow_override': _('Branch override behavior of a project cannot be changed after creation.')})


Why? Even if we know why we need to be able to tell people why.

Not sure I like this

dont' show me button if I can't do it

This is not coming up through UI, getting mysterious 400 instead:

Let me recap the reason that this was added, say someone does this:

make project with allow_override being true

makes JT with a non-blank scm_branch

changes project allow_override field back to False

Now the user has a JT that specifies a branch that we cannot apply.

Providing an outline of this case is far too much wording to put in an error message, but I agree the UI should at least show the error message from the API. Or hide it when in edit mode.

Also, this is more complicated than the other "prompts" related rejections of fields. If a JT's field is rejected because the project is configured to not allow it, we currently have no mechanism or precedent for dealing with that. We have no way of letting the user know that's broken without a lot of novel UI work.

After more discussions, I agree the API side of this can be made smoother for the user interaction.

It would be fairly easy to combine a check with this, something like JobTemplate.objects.filter(project=obj).exclude(scm_branch="").exists(), and then if both this condition is satisfied, and the allow_override value changes from its prior value, then we could give a more specific error message:

Branch override behavior of this project is currently relied on by one or more existing job templates.

Then if there are no such JTs, we can just allow the PATCH/PUT.

+1 to @kdelee's view of things and your solution seems to be a step in the right direction @AlanCoding , it seems allowing the change makes more sense for the end user -- it would also make it easier if the dependent templates could be listed, though I'm unsure of performance impact.

it would also make it easier if the dependent templates could be listed

That's possible, but depends on how.

If all you're suggesting is a list of names or primary keys in the error message, then sure. That is super easy, and IMO no performance impact that matters. But this will not be linked.

Giving primary keys would be the most concise. If the message is too long, it will overflow the popup box, because error messages are intended to be concise. But if I give primary keys, I worry that someone will say that's a bad UX. But I think handling any special UI cases here is going way overboard for a corner case.

@AlanCoding I see the issue. How about something like a warning that existing job templates use branch override behavior, and offer a confirm button that would revert them if the person really doesn't care? If that's a lot to handle and offers chaos, then just the warning would be good though.

matburt · 2019-07-08T18:38:11Z

awx/api/serializers.py

@@ -2701,7 +2706,7 @@ def get_summary_fields(self, obj):
 class JobOptionsSerializer(LabelsListMixin, BaseSerializer):

    class Meta:
-        fields = ('*', 'job_type', 'inventory', 'project', 'playbook',
+        fields = ('*', 'job_type', 'inventory', 'project', 'playbook', 'scm_branch',


This needs to be consistent... it's scm_revision elsewhere, which is more appropriate... it's more than just branch.

JobSerializer has both fields scm_branch and scm_revision. The field name scm_branch was borrowed from the Project model.

The field scm_revision is the SHA1 hash or technical specifier of the commit that a job is ran against (like "r49" for subversion). This serves the purpose of audibility in AWX / Tower so that someone can look back in time and be sure they can recover the exact version of the playbook that was ran.

scm_branch is given by a user at some point. It can be several things, and the Ansible git module goes through fallback logic where it checks if it is (1) a branch (2) a tag (3) assume it's a commit, and possibly error. The fallback logic for evaluating an scm_branch here is modeled after their process.

matburt · 2019-07-08T18:39:59Z

awx/main/models/jobs.py

@@ -396,7 +418,7 @@ def _accept_or_ignore_job_kwargs(self, **kwargs):
                # Not considered an error for manual launch, to support old
                # behavior of putting them in ignored_fields and launching anyway
                if 'prompts' not in exclude_errors:
-                    errors_dict[field_name] = _('Field is not configured to prompt on launch.').format(field_name=field_name)
+                    errors_dict[field_name] = _('Field is not configured to prompt on launch.')


matburt · 2019-07-08T18:45:12Z

awx/main/tasks.py

@@ -41,6 +43,10 @@
 # Django-CRUM
 from crum import impersonate

+# GitPython
+import git


I think I may need some help understanding what is going on in this file.... is it just Runner usage cleanup?

matburt · 2019-07-08T18:49:37Z

awx/main/tasks.py

+                except (ValueError, BadGitName):
+                    pass
+            else:
+                if git_repo.head.commit.hexsha == job.project.scm_revision:


Is this to cover the scenario where they gave a specific revision? Rather than a branch or a tag?

matburt · 2019-07-08T18:57:17Z

awx/main/tasks.py

+                project_path = p.get_project_path(check_if_exists=False)
+                git_repo = git.Repo(project_path)
+                try:
+                    commit = git_repo.commit(instance.scm_branch)


I assume this works on all ref types like tags and branches?

ryanpetrello · 2019-07-08T20:40:46Z

awx/api/serializers.py

@@ -2748,16 +2753,28 @@ def to_representation(self, obj):

    def validate(self, attrs):
        if 'project' in self.fields and 'playbook' in self.fields:
-            project = attrs.get('project', self.instance and self.instance.project or None)
+            project = attrs.get('project', self.instance.project if self.instance else None)


Yes please.

ryanpetrello · 2019-07-08T20:44:24Z

awx/main/migrations/0082_v360_job_branch_overrirde.py

+        migrations.AlterField(
+            model_name='project',
+            name='scm_update_cache_timeout',
+            field=models.PositiveIntegerField(blank=True, default=0, help_text='The number of seconds after the last project update ran that a new project update will be launched as a job dependency.'),


This only changed because the help_text got an extra space in it?

Yes, this is correct, two words ran together. I figured this is a good time to fix it because I was already migrating fields on the model.

ryanpetrello · 2019-07-08T20:49:15Z

awx/main/tasks.py

+                    logger.info('Skipping project sync for {} because commit is locally available'.format(job.log_format))
+                    needs_sync = False  # requested commit is already locally available
+                except (ValueError, BadGitName):
+                    pass


Should we put a logger.error() here so people can see in the logs that they put in an invalid ref?

jbradberry · 2019-07-16T15:15:54Z

awx/main/tasks.py

+            # manual projects are not synced, user has responsibility for that
+            needs_sync = False
+        elif not os.path.exists(project_path):
+            logger.debug('Performing fresh clone of {} on this instance.'.format(job.project))


These added logging statements here all seem like they should probably be info-level, not debug.

https://github.com/ansible/awx/pull/3882/files#diff-8f5e2fe91e83c971fc84cb35476362db

A lot of the INFO-level logs got axed en masse (almost all, really), as per the reasons there. I raised the level of some select log statements in this PR. For "Skipping project sync", I can buy INFO, because it explains the lack of a unified job. But for statements like this one, they fit in the category of normal cluster operation and that seems to fit in the DEBUG stream precedent.

I agree DEBUG is more appropriate here - this is just informational regarding work we're doing, and it's not an abnormal thing to have happen. Raising the log level will probably make the logs pretty noisy.

jakemcdermott

ui code changes look good to me
Allow JT specification and prompting for project branch #4265 (comment) should be resolved by addeff2
double display of errors (both under the field and in an alert) is more than likely a pre-existing issue with our error handling utility so I've made a github issue for it alert and field-level error sometimes shown for job template forms #4464

Update migration syntax to Django 2 fix status bug where canceled switched to error

* just like we support ansible-galaxy role install, support ansible-galaxy collection install

* ansible:devel now has ansible-galaxy collection support

Addresses some cases where collection requirements do not exist collection requirements cannot be evaluated Consolidate logic for roles and collection installs

* Similar to roles/requirements.yml sync optimization logic.

bump migration fine tune validation of project allow_override return highly custom error message Restore branch after syncs to address bugs encountered after changing scm_refspec remove unused code to determine scm_revision Check Ansible version before project update and do not install collections if Ansible version too old Add docs related to project branch override New file specific to branch override and refspec Complete docs on collections to reflect current implementation and give a folder tree example Update clustering docs related to project syncs Fix bug where git depth was ignored during the local clone from project folder to run folder Fix bug where submodules were not copied

- add refspec field to project - update refspec and branch help text on project form - add refspec field to job detail - adjust form gen and ProcessErrors to show api errors for checkbox_groups correctly - consolidate showPromptButton conditionals and fix the add/edit workflow node one for showing prompt when only branch is promptable

Additional API housekeeping, removing unused code Treat default branch as no branch provided

…dit form

softwarefactory-project-zuul · 2019-08-13T02:30:10Z

Build succeeded.

awx-api-lint : SUCCESS in 3m 35s
awx-ui-lint : SUCCESS in 3m 48s
awx-ui-next-lint : SUCCESS in 4m 18s
awx-api : SUCCESS in 8m 44s
awx-ui : SUCCESS in 5m 05s
awx-ui-next : SUCCESS in 5m 01s
awx-swagger : SUCCESS in 9m 58s
awx-detect-schema-change : FAILURE in 9m 49s (non-voting)

matburt

This lgtm

softwarefactory-project-zuul · 2019-08-14T14:56:17Z

Build succeeded (gate pipeline).

awx-api-lint : SUCCESS in 4m 40s
awx-ui-lint : SUCCESS in 4m 36s
awx-ui-next-lint : SUCCESS in 5m 03s
awx-api : SUCCESS in 10m 50s
awx-ui : SUCCESS in 6m 37s
awx-ui-next : SUCCESS in 7m 45s
awx-swagger : SUCCESS in 11m 05s
awx-detect-schema-change : FAILURE in 10m 51s (non-voting)
awx-push-new-schema : SUCCESS in 9m 40s (non-voting)

Restructuring modules so that lookup don't happen if deleting

AlanCoding changed the title ~~Branch feature phase 2~~ Allow JT specification and prompting for project branch Jul 3, 2019

AlanCoding force-pushed the branch_feature_phase_2 branch from ffe2c31 to 15418a7 Compare July 3, 2019 20:52

awxbot added type:enhancement component:api labels Jul 3, 2019

AlanCoding force-pushed the branch_feature_phase_2 branch from 15418a7 to 20867a3 Compare July 8, 2019 12:10

ryanpetrello requested review from chrismeyersfsu and matburt July 8, 2019 18:31

matburt reviewed Jul 8, 2019

View reviewed changes

ryanpetrello reviewed Jul 8, 2019

View reviewed changes

AlanCoding force-pushed the branch_feature_phase_2 branch from ce2e4ac to b73e121 Compare July 15, 2019 17:53

ryanpetrello requested a review from jbradberry July 16, 2019 13:04

jbradberry reviewed Jul 16, 2019

View reviewed changes

AlanCoding force-pushed the branch_feature_phase_2 branch from b73e121 to 1ee1645 Compare July 17, 2019 18:40

This comment has been minimized.

Sign in to view

AlanCoding force-pushed the branch_feature_phase_2 branch 2 times, most recently from 052a945 to 2894ade Compare July 18, 2019 18:49

ansible deleted a comment from softwarefactory-project-zuul bot Jul 25, 2019

jakemcdermott approved these changes Aug 12, 2019

View reviewed changes

ansible deleted a comment from softwarefactory-project-zuul bot Aug 12, 2019

ansible deleted a comment from ansible-zuul bot Aug 12, 2019

ansible deleted a comment from softwarefactory-project-zuul bot Aug 12, 2019

jakemcdermott mentioned this pull request Aug 12, 2019

alert and field-level error sometimes shown for job template forms #4464

Closed

AlanCoding and others added 13 commits August 12, 2019 22:13

Add scm_refspec field

f1f57e4

Update migration syntax to Django 2 fix status bug where canceled switched to error

collections/requirements.yml support

4be65a0

* just like we support ansible-galaxy role install, support ansible-galaxy collection install

use ansible nightly

cc6413c

* ansible:devel now has ansible-galaxy collection support

Fix bugs with discovery of collection requirements

270bd19

Addresses some cases where collection requirements do not exist collection requirements cannot be evaluated Consolidate logic for roles and collection installs

force proj sync when collections/requirements.yml

d785145

* Similar to roles/requirements.yml sync optimization logic.

working commit

13751e7

avoid unnecessary checkout, more docs content

dc5d696

fix issue with interior scope declaration eslint error

79a1dbc

Add collection setting toggle to UI

2f3aafe

Additional API housekeeping, removing unused code Treat default branch as no branch provided

remove inadverdent duplicate CreateSelect2 call from playbook on jt e…

3df476e

…dit form

Fix logic for turning off override behavior

be21a8b

AlanCoding force-pushed the branch_feature_phase_2 branch from addeff2 to be21a8b Compare August 13, 2019 02:19

matburt approved these changes Aug 14, 2019

View reviewed changes

ryanpetrello approved these changes Aug 14, 2019

View reviewed changes

AlanCoding added the mergeit label Aug 14, 2019

softwarefactory-project-zuul bot merged commit 43d816b into ansible:devel Aug 14, 2019

ryanpetrello mentioned this pull request Aug 23, 2019

Fails to detect roles/requirements.yaml #4540

Closed

AlanCoding deleted the branch_feature_phase_2 branch October 9, 2019 14:47

ryanpetrello pushed a commit to ryanpetrello/awx that referenced this pull request Apr 28, 2020

Merge pull request ansible#4265 from john-westcott-iv/release_3.7.0

12b4740

Restructuring modules so that lookup don't happen if deleting

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow JT specification and prompting for project branch #4265

Allow JT specification and prompting for project branch #4265

AlanCoding commented Jul 3, 2019

softwarefactory-project-zuul bot commented Jul 3, 2019

matburt commented Jul 8, 2019

AlanCoding commented Jul 8, 2019

matburt left a comment

matburt Jul 8, 2019

kdelee Jul 24, 2019

AlanCoding Jul 24, 2019

AlanCoding Jul 24, 2019

dsesami Jul 24, 2019

AlanCoding Jul 24, 2019

dsesami Jul 25, 2019 •

edited

Loading

matburt Jul 8, 2019

AlanCoding Jul 8, 2019

matburt Jul 8, 2019

ryanpetrello Jul 8, 2019

matburt Jul 8, 2019

matburt Jul 8, 2019

matburt Jul 8, 2019

ryanpetrello Jul 8, 2019

ryanpetrello Jul 8, 2019

AlanCoding Jul 9, 2019 •

edited

Loading

ryanpetrello Jul 8, 2019 •

edited

Loading

jbradberry Jul 16, 2019

AlanCoding Jul 30, 2019

ryanpetrello Jul 31, 2019

This comment has been minimized.

jakemcdermott left a comment •

edited

Loading

softwarefactory-project-zuul bot commented Aug 13, 2019

matburt left a comment

softwarefactory-project-zuul bot commented Aug 14, 2019

Allow JT specification and prompting for project branch #4265

Allow JT specification and prompting for project branch #4265

Conversation

AlanCoding commented Jul 3, 2019

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

softwarefactory-project-zuul bot commented Jul 3, 2019

matburt commented Jul 8, 2019

AlanCoding commented Jul 8, 2019

matburt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsesami Jul 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlanCoding Jul 9, 2019 • edited Loading

Choose a reason for hiding this comment

ryanpetrello Jul 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

jakemcdermott left a comment • edited Loading

Choose a reason for hiding this comment

softwarefactory-project-zuul bot commented Aug 13, 2019

matburt left a comment

Choose a reason for hiding this comment

softwarefactory-project-zuul bot commented Aug 14, 2019

dsesami Jul 25, 2019 •

edited

Loading

AlanCoding Jul 9, 2019 •

edited

Loading

ryanpetrello Jul 8, 2019 •

edited

Loading

jakemcdermott left a comment •

edited

Loading