Add scm_revision to project updates and cleanup

Add validation around prompted scm_branch requiring project allow_override field to be true Updated related process isolation docs Fix invalid comarision in serializer from PR review, clarify pre-check logging, minor docs additions
2026-09-16 00:40:11 -02:30 · 2019-08-12 11:01:10 -04:00
parent 76dcd57ac6
commit 6baba10abe
13 changed files with 192 additions and 83 deletions
@@ -1286,7 +1286,7 @@ class ProjectOptionsSerializer(BaseSerializer):

    class Meta:
        fields = ('*', 'local_path', 'scm_type', 'scm_url', 'scm_branch',
-                  'scm_clean', 'scm_delete_on_update', 'credential', 'timeout',)
+                  'scm_clean', 'scm_delete_on_update', 'credential', 'timeout', 'scm_revision')

    def get_related(self, obj):
        res = super(ProjectOptionsSerializer, self).get_related(obj)
@@ -1338,7 +1338,7 @@ class ProjectSerializer(UnifiedJobTemplateSerializer, ProjectOptionsSerializer):
    class Meta:
        model = Project
        fields = ('*', 'organization', 'scm_update_on_launch',
-                  'scm_update_cache_timeout', 'scm_revision', 'allow_override', 'custom_virtualenv',) + \
+                  'scm_update_cache_timeout', 'allow_override', 'custom_virtualenv',) + \
                 ('last_update_failed', 'last_updated')  # Backwards compatibility

    def get_related(self, obj):
@@ -1388,6 +1388,11 @@ class ProjectSerializer(UnifiedJobTemplateSerializer, ProjectOptionsSerializer):
        elif self.instance:
            organization = self.instance.organization

+        if 'allow_override' in attrs and self.instance:
+            if attrs['allow_override'] != self.instance.allow_override:
+                raise serializers.ValidationError({
+                    'allow_override': _('Branch override behavior of a project cannot be changed after creation.')})
+
        view = self.context.get('view', None)
        if not organization and not view.request.user.is_superuser:
            # Only allow super users to create orgless projects
@@ -2748,8 +2753,11 @@ class JobOptionsSerializer(LabelsListMixin, BaseSerializer):

    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)
            playbook = attrs.get('playbook', self.instance and self.instance.playbook or '')
+            scm_branch = attrs.get('scm_branch', self.instance.scm_branch if self.instance else None)
+            ask_scm_branch_on_launch = attrs.get(
+                'ask_scm_branch_on_launch', self.instance.ask_scm_branch_on_launch if self.instance else None)
            if not project:
                raise serializers.ValidationError({'project': _('This field is required.')})
            playbook_not_found = bool(
@@ -2763,6 +2771,10 @@ class JobOptionsSerializer(LabelsListMixin, BaseSerializer):
                raise serializers.ValidationError({'playbook': _('Playbook not found for project.')})
            if project and not playbook:
                raise serializers.ValidationError({'playbook': _('Must select playbook for project.')})
+            if scm_branch and not project.allow_override:
+                raise serializers.ValidationError({'scm_branch': _('Project does not allow overriding branch.')})
+            if ask_scm_branch_on_launch and not project.allow_override:
+                raise serializers.ValidationError({'ask_scm_branch_on_launch': _('Project does not allow overriding branch.')})

        ret = super(JobOptionsSerializer, self).validate(attrs)
        return ret
@@ -38,4 +38,9 @@ class Migration(migrations.Migration):
            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.'),
        ),
+        migrations.AddField(
+            model_name='projectupdate',
+            name='scm_revision',
+            field=models.CharField(blank=True, default='', editable=False, help_text='The SCM Revision discovered by this update for the given project and branch.', max_length=1024, verbose_name='SCM Revision'),
+        ),
    ]
@@ -101,7 +101,7 @@ class JobOptions(BaseModel):
        default='',
        blank=True,
        help_text=_('Branch to use in job run. Project default used if blank. '
-                  'Only allowed if project allow_override field is set to true.'),
+                    'Only allowed if project allow_override field is set to true.'),
    )
    forks = models.PositiveIntegerField(
        blank=True,
@@ -400,6 +400,16 @@ class JobTemplate(UnifiedJobTemplate, JobOptions, SurveyJobTemplateMixin, Resour
                # counted as neither accepted or ignored
                continue
            elif getattr(self, ask_field_name):
+                # Special case where prompts can be rejected based on project setting
+                if field_name == 'scm_branch':
+                    if not self.project:
+                        rejected_data[field_name] = new_value
+                        errors_dict[field_name] = _('Project is missing.')
+                        continue
+                    if kwargs['scm_branch'] != self.project.scm_branch and not self.project.allow_override:
+                        rejected_data[field_name] = new_value
+                        errors_dict[field_name] = _('Project does not allow override of branch.')
+                        continue
                # accepted prompt
                prompted_data[field_name] = new_value
            else:
@@ -408,7 +418,7 @@ class JobTemplate(UnifiedJobTemplate, JobOptions, SurveyJobTemplateMixin, Resour
                # 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.')

        if ('prompts' not in exclude_errors and
                (not getattr(self, 'ask_credential_on_launch', False)) and
@@ -476,6 +476,14 @@ class ProjectUpdate(UnifiedJob, ProjectOptions, JobNotificationMixin, TaskManage
        choices=PROJECT_UPDATE_JOB_TYPE_CHOICES,
        default='check',
    )
+    scm_revision = models.CharField(
+        max_length=1024,
+        blank=True,
+        default='',
+        editable=False,
+        verbose_name=_('SCM Revision'),
+        help_text=_('The SCM Revision discovered by this update for the given project and branch.'),
+    )

    def _get_parent_field_name(self):
        return 'project'
@@ -1593,17 +1593,6 @@ class RunJob(BaseTask):
        '''
        return getattr(settings, 'AWX_PROOT_ENABLED', False)

-    def copy_folders(self, project_path, galaxy_install_path, private_data_dir):
-        if project_path is None:
-            raise RuntimeError('project does not supply a valid path')
-        elif not os.path.exists(project_path):
-            raise RuntimeError('project path %s cannot be found' % project_path)
-        runner_project_folder = os.path.join(private_data_dir, 'project')
-        copy_tree(project_path, runner_project_folder)
-        if galaxy_install_path:
-            galaxy_run_path = os.path.join(private_data_dir, 'project', 'roles')
-            copy_tree(galaxy_install_path, galaxy_run_path)
-
    def pre_run_hook(self, job, private_data_dir):
        if job.inventory is None:
            error = _('Job could not start because it does not have a valid inventory.')
@@ -1620,8 +1609,6 @@ class RunJob(BaseTask):
            job = self.update_model(job.pk, status='failed', job_explanation=msg)
            raise RuntimeError(msg)

-        galaxy_install_path = None
-        git_repo = None
        project_path = job.project.get_project_path(check_if_exists=False)
        job_revision = job.project.scm_revision
        needs_sync = True
@@ -1630,21 +1617,20 @@ class RunJob(BaseTask):
            needs_sync = False
        elif not os.path.exists(project_path):
            logger.debug('Performing fresh clone of {} on this instance.'.format(job.project))
-            needs_sync = True
+        elif job.project.scm_revision:
+            logger.debug('Revision not known for {}, will sync with remote'.format(job.project))
        elif job.project.scm_type == 'git':
            git_repo = git.Repo(project_path)
-            if job.scm_branch and job.scm_branch != job.project.scm_branch and git_repo:
-                try:
-                    commit = git_repo.commit(job.scm_branch)
-                    job_revision = commit.hexsha
-                    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
-            else:
-                if git_repo.head.commit.hexsha == job.project.scm_revision:
-                    logger.info('Source tree for for {} is already up to date'.format(job.log_format))
-                    needs_sync = False
+            try:
+                desired_revision = job.project.scm_revision
+                if job.scm_branch and job.scm_branch != job.project.scm_branch:
+                    desired_revision = job.scm_branch  # could be commit or not, but will try as commit
+                commit = git_repo.commit(desired_revision)
+                job_revision = commit.hexsha
+                logger.info('Skipping project sync for {} because commit is locally available'.format(job.log_format))
+                needs_sync = False
+            except (ValueError, BadGitName):
+                logger.debug('Needed commit for {} not in local source tree, will sync with remote'.format(job.log_format))
        # Galaxy requirements are not supported for manual projects
        if not needs_sync and job.project.scm_type:
            # see if we need a sync because of presence of roles
@@ -1653,6 +1639,7 @@ class RunJob(BaseTask):
                logger.debug('Running project sync for {} because of galaxy role requirements.'.format(job.log_format))
                needs_sync = True

+        galaxy_install_path = None
        if needs_sync:
            pu_ig = job.instance_group
            pu_en = job.execution_node
@@ -1682,28 +1669,8 @@ class RunJob(BaseTask):
            try:
                sync_task = project_update_task(roles_destination=galaxy_install_path)
                sync_task.run(local_project_sync.id)
-                # if job overrided the branch, we need to find the revision that will be ran
-                if job.scm_branch and job.scm_branch != job.project.scm_branch:
-                    # TODO: handle case of non-git
-                    if job.project.scm_type == 'git':
-                        git_repo = git.Repo(project_path)
-                        try:
-                            commit = git_repo.commit(job.scm_branch)
-                            job_revision = commit.hexsha
-                            logger.debug('Evaluated {} to be a valid commit for {}'.format(job.scm_branch, job.log_format))
-                        except (ValueError, BadGitName):
-                            # not a commit, see if it is a ref
-                            try:
-                                user_branch = getattr(git_repo.refs, job.scm_branch)
-                                job_revision = user_branch.commit.hexsha
-                                logger.debug('Evaluated {} to be a valid ref for {}'.format(job.scm_branch, job.log_format))
-                            except git.exc.NoSuchPathError as exc:
-                                raise RuntimeError('Could not find specified version {}, error: {}'.format(
-                                    job.scm_branch, exc
-                                ))
-                else:
-                    job_revision = sync_task.updated_revision
-                job = self.update_model(job.pk, scm_revision=job_revision)
+                local_project_sync.refresh_from_db()
+                job = self.update_model(job.pk, scm_revision=local_project_sync.scm_revision)
            except Exception:
                local_project_sync.refresh_from_db()
                if local_project_sync.status != 'canceled':
@@ -1725,6 +1692,8 @@ class RunJob(BaseTask):
                os.mkdir(runner_project_folder)
            tmp_branch_name = 'awx_internal/{}'.format(uuid4())
            # always clone based on specific job revision
+            if not job.scm_revision:
+                raise RuntimeError('Unexpectedly could not determine a revision to run from project.')
            source_branch = git_repo.create_head(tmp_branch_name, job.scm_revision)
            git_repo.clone(runner_project_folder, branch=source_branch, depth=1, single_branch=True)
            # force option is necessary because remote refs are not counted, although no information is lost
@@ -1779,7 +1748,7 @@ class RunProjectUpdate(BaseTask):

    def __init__(self, *args, roles_destination=None, **kwargs):
        super(RunProjectUpdate, self).__init__(*args, **kwargs)
-        self.updated_revision = None
+        self.playbook_new_revision = None
        self.roles_destination = roles_destination

    def event_handler(self, event_data):
@@ -1788,7 +1757,7 @@ class RunProjectUpdate(BaseTask):
        if returned_data.get('task_action', '') == 'set_fact':
            returned_facts = returned_data.get('res', {}).get('ansible_facts', {})
            if 'scm_version' in returned_facts:
-                self.updated_revision = returned_facts['scm_version']
+                self.playbook_new_revision = returned_facts['scm_version']

    def build_private_data(self, project_update, private_data_dir):
        '''
@@ -1903,10 +1872,12 @@ class RunProjectUpdate(BaseTask):
        scm_url, extra_vars_new = self._build_scm_url_extra_vars(project_update)
        extra_vars.update(extra_vars_new)

-        if project_update.project.scm_revision and project_update.job_type == 'run' and not project_update.project.allow_override:
+        scm_branch = project_update.scm_branch
+        branch_override = bool(project_update.scm_branch != project_update.project.scm_branch)
+        if project_update.job_type == 'run' and scm_branch and (not branch_override):
            scm_branch = project_update.project.scm_revision
-        else:
-            scm_branch = project_update.scm_branch or {'hg': 'tip'}.get(project_update.scm_type, 'HEAD')
+        elif not scm_branch:
+            scm_branch = {'hg': 'tip'}.get(project_update.scm_type, 'HEAD')
        extra_vars.update({
            'project_path': project_update.get_project_path(check_if_exists=False),
            'insights_url': settings.INSIGHTS_URL_BASE,
@@ -1918,12 +1889,12 @@ class RunProjectUpdate(BaseTask):
            'scm_clean': project_update.scm_clean,
            'scm_delete_on_update': project_update.scm_delete_on_update if project_update.job_type == 'check' else False,
            'scm_full_checkout': True if project_update.job_type == 'run' else False,
-            'scm_revision': project_update.project.scm_revision,
            'roles_enabled': getattr(settings, 'AWX_ROLES_ENABLED', True) if project_update.job_type != 'check' else False
        })
+        # TODO: apply custom refspec from user for PR refs and the like
        if project_update.project.allow_override:
            # If branch is override-able, do extra fetch for all branches
-            # coming feature TODO: obtain custom refspec from user for PR refs and the like
+            # coming feature
            extra_vars['git_refspec'] = 'refs/heads/*:refs/remotes/origin/*'
        if self.roles_destination:
            extra_vars['roles_destination'] = self.roles_destination
@@ -2053,16 +2024,39 @@ class RunProjectUpdate(BaseTask):
        self.acquire_lock(instance)

    def post_run_hook(self, instance, status):
+        # TODO: find the effective revision and save to scm_revision
        self.release_lock(instance)
        p = instance.project
+        if self.playbook_new_revision:
+            instance.scm_revision = self.playbook_new_revision
+            # If branch of the update differs from project, then its revision will differ
+            if instance.scm_branch != p.scm_branch and p.scm_type == 'git':
+                project_path = p.get_project_path(check_if_exists=False)
+                git_repo = git.Repo(project_path)
+                try:
+                    commit = git_repo.commit(instance.scm_branch)
+                    instance.scm_revision = commit.hexsha  # obtain 40 char long-form of SHA1
+                    logger.debug('Evaluated {} to be a valid commit for {}'.format(instance.scm_branch, instance.log_format))
+                except (ValueError, BadGitName):
+                    # not a commit, see if it is a ref
+                    try:
+                        user_branch = getattr(git_repo.remotes.origin.refs, instance.scm_branch)
+                        instance.scm_revision = user_branch.commit.hexsha  # head of ref
+                        logger.debug('Evaluated {} to be a valid ref for {}'.format(instance.scm_branch, instance.log_format))
+                    except (git.exc.NoSuchPathError, AttributeError) as exc:
+                        raise RuntimeError('Could not find specified version {}, error: {}'.format(
+                            instance.scm_branch, exc
+                        ))
+            instance.save(update_fields=['scm_revision'])
        if instance.job_type == 'check' and status not in ('failed', 'canceled',):
-            if self.updated_revision:
-                p.scm_revision = self.updated_revision
+            if self.playbook_new_revision:
+                p.scm_revision = self.playbook_new_revision
            else:
-                logger.info("{} Could not find scm revision in check".format(instance.log_format))
+                if status == 'successful':
+                    logger.error("{} Could not find scm revision in check".format(instance.log_format))
            p.playbook_files = p.playbooks
            p.inventory_files = p.inventories
-            p.save()
+            p.save(update_fields=['scm_revision', 'playbook_files', 'inventory_files'])

        # Update any inventories that depend on this project
        dependent_inventory_sources = p.scm_inventory_sources.filter(update_on_project_update=True)
@@ -516,6 +516,25 @@ def test_job_launch_JT_with_credentials(machine_credential, credential, net_cred
    assert machine_credential in creds


+@pytest.mark.django_db
+def test_job_branch_rejected_and_accepted(deploy_jobtemplate):
+    deploy_jobtemplate.ask_scm_branch_on_launch = True
+    deploy_jobtemplate.save()
+    prompted_fields, ignored_fields, errors = deploy_jobtemplate._accept_or_ignore_job_kwargs(
+        scm_branch='foobar'
+    )
+    assert 'scm_branch' in ignored_fields
+    assert 'does not allow override of branch' in errors['scm_branch']
+
+    deploy_jobtemplate.project.allow_override = True
+    deploy_jobtemplate.project.save()
+    prompted_fields, ignored_fields, errors = deploy_jobtemplate._accept_or_ignore_job_kwargs(
+        scm_branch='foobar'
+    )
+    assert not ignored_fields
+    assert prompted_fields['scm_branch'] == 'foobar'
+
+
@pytest.mark.django_db
@pytest.mark.job_runtime_vars
 def test_job_launch_unprompted_vars_with_survey(mocker, survey_spec_factory, job_template_prompts, post, admin_user):
@@ -505,3 +505,37 @@ def test_callback_disallowed_null_inventory(project):
    with pytest.raises(ValidationError) as exc:
        serializer.validate({'host_config_key': 'asdfbasecfeee'})
    assert 'Cannot enable provisioning callback without an inventory set' in str(exc)
+
+
+@pytest.mark.django_db
+def test_job_template_branch_error(project, inventory, post, admin_user):
+    r = post(
+        url=reverse('api:job_template_list'),
+        data={
+            "name": "fooo",
+            "inventory": inventory.pk,
+            "project": project.pk,
+            "playbook": "helloworld.yml",
+            "scm_branch": "foobar"
+        },
+        user=admin_user,
+        expect=400
+    )
+    assert 'Project does not allow overriding branch' in str(r.data['scm_branch'])
+
+
+@pytest.mark.django_db
+def test_job_template_branch_prompt_error(project, inventory, post, admin_user):
+    r = post(
+        url=reverse('api:job_template_list'),
+        data={
+            "name": "fooo",
+            "inventory": inventory.pk,
+            "project": project.pk,
+            "playbook": "helloworld.yml",
+            "ask_scm_branch_on_launch": True
+        },
+        user=admin_user,
+        expect=400
+    )
+    assert 'Project does not allow overriding branch' in str(r.data['ask_scm_branch_on_launch'])
@@ -10,12 +10,12 @@ from awx.api.versioning import reverse
@pytest.mark.django_db
 class TestInsightsCredential:
    def test_insights_credential(self, patch, insights_project, admin_user, insights_credential):
-        patch(insights_project.get_absolute_url(), 
+        patch(insights_project.get_absolute_url(),
              {'credential': insights_credential.id}, admin_user,
              expect=200)

    def test_non_insights_credential(self, patch, insights_project, admin_user, scm_credential):
-        patch(insights_project.get_absolute_url(), 
+        patch(insights_project.get_absolute_url(),
              {'credential': scm_credential.id}, admin_user,
              expect=400)

@@ -44,3 +44,24 @@ def test_project_unset_custom_virtualenv(get, patch, project, admin, value):
    url = reverse('api:project_detail', kwargs={'pk': project.id})
    resp = patch(url, {'custom_virtualenv': value}, user=admin, expect=200)
    assert resp.data['custom_virtualenv'] is None
+
+
+@pytest.mark.django_db
+def test_no_changing_overwrite_behavior(post, patch, organization, admin_user):
+    r1 = post(
+        url=reverse('api:project_list'),
+        data={
+            'name': 'fooo',
+            'organization': organization.id,
+            'allow_override': True
+        },
+        user=admin_user,
+        expect=201
+    )
+    r2 = patch(
+        url=reverse('api:project_detail', kwargs={'pk': r1.data['id']}),
+        data={'allow_override': False},
+        user=admin_user,
+        expect=400
+    )
+    assert 'cannot be changed' in str(r2.data['allow_override'])
@@ -256,7 +256,7 @@ class TestExtraVarSanitation(TestJobExecution):

    def test_vars_unsafe_by_default(self, job, private_data_dir):
        job.created_by = User(pk=123, username='angry-spud')
-        job.inventory = Inventory(pk=123, name='example-inv') 
+        job.inventory = Inventory(pk=123, name='example-inv')

        task = tasks.RunJob()
        task.build_extra_vars_file(job, private_data_dir)
@@ -367,10 +367,10 @@ class TestGenericRun():
        task = tasks.RunJob()
        task.update_model = mock.Mock(return_value=job)
        task.build_private_data_files = mock.Mock(side_effect=OSError())
-        task.copy_folders = mock.Mock()

-        with pytest.raises(Exception):
-            task.run(1)
+        with mock.patch('awx.main.tasks.copy_tree'):
+            with pytest.raises(Exception):
+                task.run(1)

        update_model_call = task.update_model.call_args[1]
        assert 'OSError' in update_model_call['result_traceback']
@@ -386,10 +386,10 @@ class TestGenericRun():
        task = tasks.RunJob()
        task.update_model = mock.Mock(wraps=update_model_wrapper)
        task.build_private_data_files = mock.Mock()
-        task.copy_folders = mock.Mock()

-        with pytest.raises(Exception):
-            task.run(1)
+        with mock.patch('awx.main.tasks.copy_tree'):
+            with pytest.raises(Exception):
+                task.run(1)

        for c in [
            mock.call(1, status='running', start_args=''),
@@ -1722,8 +1722,6 @@ class TestProjectUpdateCredentials(TestJobExecution):
        call_args, _ = task._write_extra_vars_file.call_args_list[0]
        _, extra_vars = call_args

-        assert extra_vars["scm_revision_output"] == 'foobar'
-
    def test_username_and_password_auth(self, project_update, scm_type):
        task = tasks.RunProjectUpdate()
        ssh = CredentialType.defaults['ssh']()
@@ -11,7 +11,6 @@
 # scm_username: username (only for svn/insights)
 # scm_password: password (only for svn/insights)
 # scm_accept_hostkey: true/false (only for git, set automatically)
-# scm_revision: current revision in tower
 # git_refspec: a refspec to fetch in addition to obtaining version
 # roles_enabled: Allow us to pull roles from a requirements.yml file
 # roles_destination: Path to save roles from galaxy to