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

Allow dashes in legacy role namespaces #3962

Merged
merged 2 commits into from
Jan 11, 2024
Merged

Conversation

sur5r
Copy link
Contributor

@sur5r sur5r commented Dec 28, 2023

Fixes: #3961

@sur5r sur5r marked this pull request as ready for review December 29, 2023 13:33
@sur5r sur5r requested a review from a team as a code owner December 29, 2023 13:33
@sur5r sur5r requested review from ssbarnea and shatakshiiii and removed request for a team December 29, 2023 13:33
@ssbarnea
Copy link
Member

Can you please also update the link to documentation to a valid page that documents the new namespace requirements?

Copy link
Member

@ssbarnea ssbarnea left a comment

Choose a reason for hiding this comment

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

This might not be enough to allow dashes in namespaces but we can probably add some tests ourselfs. Still, before this, please include links to documentation that includes the new namespace requirements as we do not want to just rely on "current behavior".

@sur5r
Copy link
Contributor Author

sur5r commented Dec 29, 2023

Unfortunately, I have no idea where this is documented.

I have to admit that I have spent way too many hours fighting windmills around this galaxy vs. galaxy-ng vs. namespaces vs. legacy namespaces vs. dash-to-underscore-to-dash-to-dot-conversion disaster.

I fully understand if you don't want to merge this without proper docs and or tests, but I'm completely lost here.

@jctanner
Copy link

jctanner commented Jan 8, 2024

It's not documented in the narrative style but it is encoded into galaxy-importer's legacy role validation code.

https://github.com/ansible/galaxy-importer/blob/master/galaxy_importer/loaders/legacy_role.py#L57-L61
https://github.com/ansible/galaxy-importer/blob/master/galaxy_importer/constants.py#L56C59-L60

Galaxy must support every possible github username as a namespace name for legacy roles. This is because galaxy was originally meant to be an index of github, not an artifactory competitor. It was collections that started morphing galaxy into something different from it's original intent and once the team started trying to fudge all the constraints of collections onto roles that things got fuzzy. That is why at some point they attempted to convert all namespace hyphens to underscores, which turned out to be a big mistake. Now we're left with a mix of role namespaces that don't quite map to their github usernames. GalaxyNG's implementation sought to eliminate the confusion by separating collection and role namespaces. Now the role namespaces can continue to have hyphens (and underscores because of the historical mistake) and map to the true github usernames as they were always meant to. Collection namespaces can not have hyphens and will never be able to because of how the ansible-core architecture uses them via python imports.

In summary, this change is good as long as it's only changing it for standalone/legacy roles.

@ssbarnea ssbarnea changed the title Allow dashes in namespaces Allow dashes in legacy role namespaces Jan 10, 2024
@ssbarnea
Copy link
Member

@sur5r You failed to update a fixture and it seems that I am not allowed to push the fix to your branch. There is a checkbox in the right sidebar for allowing others to push to your PR.

@sur5r
Copy link
Contributor Author

sur5r commented Jan 10, 2024

The "Allow edits by maintainers" checkbox is checked. I cherry-picked your commit onto my branch. Thank you very much for moving this forward!

@ssbarnea ssbarnea merged commit 974c54d into ansible:main Jan 11, 2024
23 checks passed
nrdufour added a commit to nrdufour/home-ops that referenced this pull request Jan 20, 2024
This PR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [ansible-lint](https://github.com/ansible/ansible-lint) ([changelog](https://github.com/ansible/ansible-lint/releases)) | patch | `==6.22.1` -> `==6.22.2` |

---

### Release Notes

<details>
<summary>ansible/ansible-lint (ansible-lint)</summary>

### [`v6.22.2`](https://github.com/ansible/ansible-lint/releases/tag/v6.22.2)

[Compare Source](ansible/ansible-lint@v6.22.1...v6.22.2)

#### Bugfixes

-   Fix key error for name\[casing] rule ([#&#8203;3987](ansible/ansible-lint#3987)) [@&#8203;ajinkyau](https://github.com/ajinkyau)
-   Allow dashes in legacy role namespaces ([#&#8203;3962](ansible/ansible-lint#3962)) [@&#8203;sur5r](https://github.com/sur5r)
-   Use new ansible-compat verbosity levels ([#&#8203;3975](ansible/ansible-lint#3975)) [@&#8203;ssbarnea](https://github.com/ssbarnea)
-   Remove dependency on newer requests library ([#&#8203;3959](ansible/ansible-lint#3959)) [@&#8203;ssbarnea](https://github.com/ssbarnea)
-   Ignore set-property for systemd command ([#&#8203;3949](ansible/ansible-lint#3949)) [@&#8203;alanbbr](https://github.com/alanbbr)
-   Correct requires_ansible error message ([#&#8203;3954](ansible/ansible-lint#3954)) [@&#8203;ssbarnea](https://github.com/ssbarnea)
-   Improve transformation for `no-free-form` rule ([#&#8203;3945](ansible/ansible-lint#3945)) [@&#8203;ajinkyau](https://github.com/ajinkyau)
-   Documentation improvement ([#&#8203;3946](ansible/ansible-lint#3946)) [@&#8203;ssbarnea](https://github.com/ssbarnea)
-   docs: fix grammatical issue in philosophy Q\&A section ([#&#8203;3934](ansible/ansible-lint#3934)) [@&#8203;davidhulick](https://github.com/davidhulick)
-   Update supported versions of ansible ([#&#8203;3930](ansible/ansible-lint#3930)) [@&#8203;ajinkyau](https://github.com/ajinkyau)
-   Fix backward compatibility ([#&#8203;3929](ansible/ansible-lint#3929)) [@&#8203;McSim85](https://github.com/McSim85)
-   Fix auto capitalization for name\[prefix] rule ([#&#8203;3922](ansible/ansible-lint#3922)) [@&#8203;ajinkyau](https://github.com/ajinkyau)
-   Fix role deps check for detecting path names ([#&#8203;3923](ansible/ansible-lint#3923)) [@&#8203;cavcrosby](https://github.com/cavcrosby)
-   Avoid warnings about PATH with pipx installations ([#&#8203;3920](ansible/ansible-lint#3920)) [@&#8203;ssbarnea](https://github.com/ssbarnea)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4xNDAuMCIsInVwZGF0ZWRJblZlciI6IjM3LjE0MC4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->

Reviewed-on: https://git.home/nrdufour/home-ops/pulls/319
Co-authored-by: Renovate <[email protected]>
Co-committed-by: Renovate <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Fails to accept namespace containing dash
3 participants