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

Fixes #37988 - migrate container repo naming to use slashes #11208

Merged

Conversation

ianballou
Copy link
Member

What are the changes introduced in this pull request?

Changes the default container naming scheme to use slashes instead of dashes.

Considerations taken when implementing this change?

We don't want the change to happen for existing repositories. To do this, we rely on the fact that the ::Katello::Repository model holds the container repository name, which gets changed on updates as a before_validation hook. When updating repositories in the UI, that changes the root, not the repository. So, there is no way for the container repository to be update just by updating things on the UI.

However, the naming scheme will change for content view publishes and promotions, since the environment repository gets updated.

Is this behavior okay? Should new content views with old repositories continue to use the old naming default? In a way, when publishing/promoting, it's like creating new repositories. If we need to keep the old default, it may complicate the logic a bit.

Plus, we want users to use this new naming scheme because it is better and more readable. If users don't like the new scheme, it's easy to go back to the old default by adding a custom registry naming scheme.

What are the testing steps for this pull request?

  1. Create some container repos
  2. Publish those repos to some CVs in multiple environments
  3. Add in my change
  4. Check that existing repositories use the same old scheme, even if they get updated
  5. Check that the existing CV version container repos have the same old naming scheme
  6. Check that promoting or publishing the CVs updates the naming scheme
  7. Check that new repositories use the new naming scheme.

@ianballou ianballou force-pushed the 37988-migrate-container-repo-naming-scheme branch 3 times, most recently from c09712b to 00e54eb Compare November 13, 2024 19:34
@sjha4
Copy link
Member

sjha4 commented Nov 20, 2024

Code looks good. Starting to test. Does it make sense to edit the description of the pattern in katello/engines/bastion_katello/app/assets/javascripts/bastion_katello/environments/details/views/environment-details.html to something like
By default this name is a / separated string of Organization, Lifecycle Environment, Content View, Product, and Repository labels.

@ianballou
Copy link
Member Author

Code looks good. Starting to test. Does it make sense to edit the description of the pattern in katello/engines/bastion_katello/app/assets/javascripts/bastion_katello/environments/details/views/environment-details.html to something like By default this name is a / separated string of Organization, Lifecycle Environment, Content View, Product, and Repository labels.

I'll take a look, I think that makes sense if it'll fit.

@sjha4
Copy link
Member

sjha4 commented Nov 21, 2024

Able to see updated registry names for the repositories on this PR. 👍🏼

@ianballou ianballou force-pushed the 37988-migrate-container-repo-naming-scheme branch from 00e54eb to 5d92e90 Compare November 21, 2024 21:43
@ianballou
Copy link
Member Author

@sjha4 I updated the help text.

Copy link
Member

@sjha4 sjha4 left a comment

Choose a reason for hiding this comment

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

Ack.. 👍🏼

@ianballou
Copy link
Member Author

Test failure unrelated and the other pipelines passed, merging!

@ianballou ianballou merged commit 9c98533 into Katello:master Nov 22, 2024
24 of 27 checks passed
@ianballou ianballou deleted the 37988-migrate-container-repo-naming-scheme branch November 22, 2024 16:04
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.

2 participants