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 #36436 - Don't sync generated repos to capsules #10572

Merged
merged 1 commit into from
Jun 5, 2023

Conversation

sjha4
Copy link
Member

@sjha4 sjha4 commented May 25, 2023

What are the changes introduced in this pull request?

Opening this up for reviews.

Considerations taken when implementing this change?

I am not so sure if we need to ignore all generated CV repos. That's what I have here but if we want to narrow it down to all exported repos only, I can make that change.

What are the testing steps for this pull request?

In hammer do a repo export, library export, import and other operations.
These generated repos should not get synced to the capsule tied to the environment or library.

@theforeman-bot
Copy link

Issues: #36436

@sjha4 sjha4 requested a review from parthaa May 25, 2023 17:07
@sjha4 sjha4 force-pushed the skip_exported_repos_on_capsule_sync branch from 0663e97 to d03fefe Compare May 31, 2023 16:32
Copy link
Member

@ianballou ianballou left a comment

Choose a reason for hiding this comment

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

This is working fine for me! I'm not seeing any of the generated repositories get synced to the capsule.

Copy link
Member

@ianballou ianballou left a comment

Choose a reason for hiding this comment

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

I'm cool with this!

Copy link
Member

@chris1984 chris1984 left a comment

Choose a reason for hiding this comment

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

I think having a setting to toggle the importable content view would be best and solve the case of preventing the user from synchronizing it to the capsule and for a scenario like a customer can't physically get to the capsule to import the files, edge cases, then they can enable the setting and have it sync.

@ianballou
Copy link
Member

a scenario like a customer can't physically get to the capsule to import the files, edge cases, then they can enable the setting and have it sync.

With how import/export is, can a customer do anything with the generated repos synced to a capsule? We'd need to make sure that use case is documented so users don't get more confused.

Also, from the bug description on the Redmine, it sounds like the certs can get messed up.

:repository_import,
:library_export_syncable,
:repository_export_syncable])
scope :ignore_generated, ->(include_library_generated = false) {
Copy link
Contributor

@parthaa parthaa Jun 1, 2023

Choose a reason for hiding this comment

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

Shouldn't this be

Suggested change
scope :ignore_generated, ->(include_library_generated = false) {
scope :ignore_generated, ->(include_library_generated: false) {

@@ -32,7 +37,7 @@ def clear_smart_proxy_sync_histories(repo_list = [])

def combined_repos_available_to_capsule(environment = nil, content_view = nil, repository = nil)
lifecycle_environment_check(environment, repository)
if repository
if repository && !(library_export_repo repository)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if repository && !(library_export_repo repository)
if repository && !(library_export_repo(repository))

@parthaa
Copy link
Contributor

parthaa commented Jun 1, 2023

LGTM other than the 2 suggested changes

@sjha4 sjha4 force-pushed the skip_exported_repos_on_capsule_sync branch from d03fefe to 518dff5 Compare June 2, 2023 13:34
:repository_import,
:library_export_syncable,
:repository_export_syncable])
scope :ignore_generated, ->(include_library_generated: false) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a simple model unit test for this scope (just ensure the params are getting set correctly)

@sjha4
Copy link
Member Author

sjha4 commented Jun 2, 2023

Pushed the update.
To recap:
All generated CVs will be excluded from Capsule syncs.
Imported content should be added to a regular content view and follow regular lifecycle paths of publish/promote to make it to a capsule.

@sjha4 sjha4 force-pushed the skip_exported_repos_on_capsule_sync branch from 518dff5 to fc2f998 Compare June 5, 2023 19:54
@sjha4 sjha4 force-pushed the skip_exported_repos_on_capsule_sync branch from fc2f998 to d874dd9 Compare June 5, 2023 19:57
@parthaa
Copy link
Contributor

parthaa commented Jun 5, 2023

ACK

@sjha4 sjha4 merged commit 70c4a6f into Katello:master Jun 5, 2023
5 checks passed
wbclark pushed a commit to wbclark/katello that referenced this pull request Jun 6, 2023
wbclark pushed a commit that referenced this pull request Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants