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.

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

Remove deprecated field update_on_project_update #12366

Merged

Conversation

fosterseth
Copy link
Member

@fosterseth fosterseth commented Jun 13, 2022

SUMMARY

remove deprecated fields update_on_project_update and scm_last_revision for InventorySource
related #12206

todo: fix up / remove QA tests

ISSUE TYPE
  • New or Enhanced Feature
COMPONENT NAME
  • API
AWX VERSION
awx: 21.0.1.dev219+g0df3c655b9

@github-actions github-actions bot added component:api component:awx_collection issues related to the collection for controlling AWX component:docs labels Jun 13, 2022
proj = src.source_project
if proj and proj.update_on_launch == False:
proj.update_on_launch = True
proj.save(update_fields=['update_on_launch'])
Copy link
Member

Choose a reason for hiding this comment

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

This is the only change that might be somewhat consequential to the user. I would probably put a warning level log here. This is also a big item for the release notes.

@AlanCoding
Copy link
Member

We may be able to remove the SCM launch type

('scm', _('SCM Update')), # Job was created as an Inventory SCM sync.

If not, I would advocate that we change it to deprecated. Inventory updates from before the migration could still validly have this status.

@AlanCoding
Copy link
Member

Even if we do nothing on that launch_type option, we can still purge references to the "scm" value. In particular, these lines in the RunInventorySource model are complementary to logic in the _update_dependent_inventories which is already being deleted.

awx/awx/main/tasks/jobs.py

Lines 1604 to 1606 in 0df3c65

elif inventory_update.source == 'scm' and inventory_update.launch_type == 'scm' and source_project:
# This follows update, not sync, so make copy here
RunProjectUpdate.make_local_copy(source_project, private_data_dir)

@AlanCoding
Copy link
Member

A more minor deletion would be here:

if self.launch_type != 'scm' and self.source_project_update:

The condition self.launch_type != 'scm' should always resolve to true after this. So only checking if self.source_project_update_id: would be the most efficient.

@@ -1655,8 +1591,6 @@ def pre_run_hook(self, inventory_update, private_data_dir):
sync_task = project_update_task(job_private_data_dir=private_data_dir)
sync_task.run(local_project_sync.id)
local_project_sync.refresh_from_db()
inventory_update.inventory_source.scm_last_revision = local_project_sync.scm_revision
inventory_update.inventory_source.save(update_fields=['scm_last_revision'])
Copy link
Member

Choose a reason for hiding this comment

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

This looks like it may need something added back in. Jobs have an scm_revision field to track which revision it ran. We may want to add back in a similar field for inventory updates.

Copy link
Member Author

@fosterseth fosterseth Jun 14, 2022

Choose a reason for hiding this comment

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

would it be better to have InventorySource.scm_revision to be consistent with Job.scm_revision?

Copy link
Member

Choose a reason for hiding this comment

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

I'm hitting some similar questions in #12356. Jobs and inventory updates logically should have a multitude of things in common (like this) but it was never fully thought out. My short answer is "yes", it should be consistent, but the broader UI impact and things probably need to get some thought.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:api component:awx_collection issues related to the collection for controlling AWX component:docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants