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 vendored inventory scripts #6911

Merged
merged 10 commits into from
Jun 18, 2020

Conversation

AlanCoding
Copy link
Member

@AlanCoding AlanCoding commented May 1, 2020

updated version of #6088


For WIP major remaining work item is figuring our a solution for existing cloudforms sources. It's possible to drop in a custom script as a replacement... but that takes work.

@ryanpetrello
Copy link
Contributor

ryanpetrello commented May 1, 2020

image

@AlanCoding
Copy link
Member Author

I've pondered on the subject, and I see the fastest way to getting this in to be just writing a migration for cloudforms. I would target the SCM inventory, url, and branch:

https://github.com/ansible/ansible/blob/stable-2.9/contrib/inventory/cloudforms.py

because I'm not deleting these just to re-vendor them in a migration. Ping @wenottingham as simply an FYI. I can't see that migration going any other way, it's not going to be super technically challenging to add it, it's just a little more I'm planning on here.

It's going to involve some nastiness with creating a new project just for this purpose, but that's fine by me.

@wenottingham
Copy link
Contributor

I've pondered on the subject, and I see the fastest way to getting this in to be just writing a migration for cloudforms. I would target the SCM inventory, url, and branch:

Is that better or worse than migrating it into the database as a deprecated script?

@AlanCoding
Copy link
Member Author

But then I would have to put a copy of the deprecated script into the migration folder, and I don't want to do that, because that's still vendoring it. I'll have to mess around with the project creation, naming, initial role creation, figuring out the organization for it, and so on. We would still have some of those problems doing a "custom script", and on-balance I would much prefer the SCM inventory. Also, custom scripts are deprecated...

@AlanCoding AlanCoding changed the title [WIP] Remove vendored inventory scripts Remove vendored inventory scripts May 15, 2020
@AlanCoding AlanCoding marked this pull request as ready for review May 15, 2020 11:14
@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@AlanCoding
Copy link
Member Author

I'm finished poking at this, testing looked okay last I checked but was conflated with other unrelated issues in devel.

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

Copy link
Member

@kdelee kdelee left a comment

Choose a reason for hiding this comment

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

🙏

@softwarefactory-project-zuul
Copy link
Contributor

Build failed.

@AlanCoding
Copy link
Member Author

recheck

for inv_src in InventorySource.objects.filter(source=source).iterator():
inv_src.source = 'scm'
inv_src.source_project = project
inv_src.source_path = 'contrib/inventory/{}.py'.format(source)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is sneaky as hell, and I highly approve of it 😄

@@ -11,10 +11,6 @@
import os.path
from urllib.parse import urljoin
import yaml
import configparser
Copy link
Contributor

@ryanpetrello ryanpetrello Jun 17, 2020

Choose a reason for hiding this comment

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

I cannot express with words the amount of joy this specific file diff brings me.

@softwarefactory-project-zuul
Copy link
Contributor

Build failed.

organization=inventory.organization,
source='ec2'
)
invsrc.create_scm_script_substitute(apps, 'ec2')
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

Copy link
Member

@kdelee kdelee left a comment

Choose a reason for hiding this comment

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

Upgrade test went smooth

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded (gate pipeline).

@softwarefactory-project-zuul softwarefactory-project-zuul bot merged commit 1321d29 into ansible:devel Jun 18, 2020
@ryanpetrello
Copy link
Contributor

ryanpetrello commented Jun 18, 2020

@AlanCoding can you open a PR with a succinct description of this (and the new requirement of 2.9+) into the CHANGELOG.md as a basis of our messaging in the next AWX release?

I think this PR merging means we're mostly done with this work item, but I think there's also going to be some clear documentation that needs to come out of this to call this truly "finished".

cc @chrismeyersfsu @tvo318

@AlanCoding
Copy link
Member Author

Let's move that discussion over to #7379

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants