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

support legacy role import overrides for namespace and name. #2011

Merged
merged 1 commit into from
Dec 21, 2023

Conversation

jctanner
Copy link
Collaborator

@jctanner jctanner commented Dec 12, 2023

https://issues.redhat.com/browse/AAH-3002

The old API supported a request parameter for alternate-role-name for a period of time, but was ultimately removed in ansible/galaxy@1d1519f

We added support in the galaxy-importer legacy role schema for the namespace name per ansible/galaxy-importer@98e0931

"role_name" was added to the initial legacy role schema per ansible/galaxy-importer@1927cd6

This PR adds a few things ....

  1. The namespace and role_name in meta/main.yml now causes the role import code to attempt to put the role by the given name into the given namespace name. RBAC still requires the github_user to map to some known namespace name that the request user is an owner of.

  2. The import API endpoint now accepts alternate_namespace_name and alternate_role_name with both overriding any values found in the role's metadata. The ansible-galaxy CLI only supports alternate_role_name but I envision these new parametes to be primarily used by the galaxy UI for UX driven imports. I'd expect CLI users to use the meta/main.yml definitions instead.

The "magic" role finding code that attempts to map a non-matching github_user to an old role with a non-matching namespace name will remain in place for now. I think after we establish this new path as the standard way of doing things, we can do away with the "magic" and make the code predicatable.

@jctanner jctanner marked this pull request as draft December 12, 2023 19:15
@github-actions github-actions bot added backport-4.2 This PR should be backported to stable-4.2 (1.2) backport-4.4 This PR should be backported to stable-4.4 (2.1) backport-4.5 This PR should be backported to stable-4.5 (2.2) backport-4.6 This PR should be backported to stable-4.6 (2.3) backport-4.7 This PR should be backported to stable-4.7 (2.4) backport-4.8 This PR should be backported to stable-4.8 (2.4) backport-4.9 This PR should be backported to stable-4.9 (2.4) labels Dec 12, 2023
@jctanner jctanner removed backport-4.2 This PR should be backported to stable-4.2 (1.2) backport-4.4 This PR should be backported to stable-4.4 (2.1) backport-4.5 This PR should be backported to stable-4.5 (2.2) backport-4.6 This PR should be backported to stable-4.6 (2.3) backport-4.7 This PR should be backported to stable-4.7 (2.4) backport-4.8 This PR should be backported to stable-4.8 (2.4) backport-4.9 This PR should be backported to stable-4.9 (2.4) labels Dec 12, 2023
@jctanner jctanner force-pushed the LEGACY_ROLE_IMPORT_OVERRIDES branch 3 times, most recently from 21187ce to bcbd3a8 Compare December 13, 2023 02:08
@jctanner jctanner changed the title Initial refactor to support overrides on namespace and name. support legacy role import overrides for namespace and name. Dec 13, 2023
@jctanner jctanner force-pushed the LEGACY_ROLE_IMPORT_OVERRIDES branch 2 times, most recently from 88847d7 to adab18c Compare December 18, 2023 00:05
@jctanner jctanner marked this pull request as ready for review December 18, 2023 01:17
@jctanner
Copy link
Collaborator Author

jctanner commented Dec 18, 2023

@resmo @geerlingguy @bcoca @sivel @s-hertel any feedback on this would be appreciated.

@resmo
Copy link

resmo commented Dec 18, 2023

Unfortunately I don't have permission to view AAH-3002 but I filed https://issues.redhat.com/browse/AAH-2861

I am convinced, this PR would solve my issue.

@himdel
Copy link
Collaborator

himdel commented Dec 18, 2023

LGTM, sorry for the merge conflict (#2012)
Updating UI accordingly in ansible/ansible-hub-ui#4763
And updated permissions on AAH-3002 :)

@himdel
Copy link
Collaborator

himdel commented Dec 18, 2023

@jctanner Any easy way to get the namespace & name info into the import summary fields? (Not only when alternate_.. are provided.)

UI can query the role by role_id to get them, but if they're easy to add here, the import list could also show them.

@s-hertel
Copy link

I like it. I don't know if I'd expect CLI users to prefer the meta/main.yml - using CLI args would make the role more portable. A ansible-galaxy role requirement can include src and name to rename the src, but role contents are not modified so hardcoded references to itself would need to be manually updated. I wouldn't be opposed to a --namespace option for ansible-galaxy role import if a github user can map to multiple role namespaces.

No-Issue

Signed-off-by: James Tanner <[email protected]>
@jctanner
Copy link
Collaborator Author

@s-hertel that's true about preferring the CLI, but my expectation is that it'll take a lot longer to get a --namespace flag into an ansible-galaxy version that most people are using.

@jctanner
Copy link
Collaborator Author

@jctanner Any easy way to get the namespace & name info into the import summary fields? (Not only when alternate_.. are provided.)

UI can query the role by role_id to get them, but if they're easy to add here, the import list could also show them.

@himdel are you referring to the response from the POST to /api/v1/imports?

@himdel
Copy link
Collaborator

himdel commented Dec 19, 2023

I meant for the import list GET, though both would be even better :).

But thinking about it more, when reading names from the role metadata, we wouldn't really know the names until the import is done, right? Same reason why role_id is null for failed imports.

I have the current imports list show the github user/repo now, because those are always available, maybe that's ok as is :).

@jctanner
Copy link
Collaborator Author

@himdel correct, we have a chicken&egg scenario on the role_id,namespace,name ... we don't know the truth to any of that until the background task finishes.

I think what you have now for listing imports is probably going to be best we can do. I still think github_user/github_repo should be the primary key but all the community feedback suggests otherwise.

@jctanner jctanner merged commit 9b44374 into ansible:master Dec 21, 2023
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants