-
Notifications
You must be signed in to change notification settings - Fork 297
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 #37795 - set multiple Content Views via a single Activation key #11135
Conversation
569088f
to
e620689
Compare
733509e
to
1030745
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @parthaa!
Some initial code comments
Also tested a bit and found that when I use the Angular page to edit syspurpose values on an AK, multiple CVEs are overwritten. This does not happen with the new React AK page.
app/lib/katello/validators/content_view_environment_org_validator.rb
Outdated
Show resolved
Hide resolved
9742ae7
to
c24c8b3
Compare
self.table_name = 'katello_activation_keys' | ||
end | ||
|
||
class CVEAKMigrator # used in db/migrate/20220929204746_add_content_view_environment_content_facet.rb |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed please change the logic here so that
if the AK doesn't have CV or LCE, don't add it. (AKs aren't required to have cv/lce.)
if the AK has only CV, add it as Default LCE and that CV (and add a warning in the docs)
if the AK has only LCE, add it as Default LCE / Default Org View
if the AK has both, great, add that CVE
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be addressed in the latest commit
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those CVEs should be part of the aks_with_no_cve + aks_with_missing_cve
list. Seems to be ok to me? I am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did some more testing
Global Reg with multiple AKs works as expected:
- Multiple AKs' CVEs are combined
- Ordering of CVEs is preserved between multiple AKs
Available Releases works as expected (Release version dropdown on AK details shows release versions from the AK's repositories)
self.table_name = 'katello_activation_keys' | ||
end | ||
|
||
class CVEAKMigrator # used in db/migrate/20220929204746_add_content_view_environment_content_facet.rb |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
I guess the only one I don't see covered is if the AK has only CV, add it as Default LCE and that CV (and add a warning in the docs) Right now it would get Library/Default Org View. |
in the first section |
c16d6b5
to
d92012e
Compare
c80cac1
to
8c233fb
Compare
412cb45
to
ec21c27
Compare
ec21c27
to
296de3a
Compare
Ruby test failures related, should be easy fixes |
296de3a
to
89677da
Compare
add_foreign_key "katello_activation_keys", "katello_environments", | ||
:name => "katello_activation_keys_environment_id", :column => "environment_id" | ||
|
||
::Katello::ActivationKey.reset_column_information |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
== 20240730163043 AddContentViewEnvironmentActivationKey: reverting ===========
-- add_column(:katello_activation_keys, :content_view_id, :integer, {:index=>true})
-> 0.0010s
-- add_column(:katello_activation_keys, :environment_id, :integer, {:index=>true})
-> 0.0004s
-- add_foreign_key("katello_activation_keys", "katello_content_views", {:name=>"katello_activation_keys_content_view_id", :column=>"content_view_id"})
-> 0.0011s
-- add_foreign_key("katello_activation_keys", "katello_environments", {:name=>"katello_activation_keys_environment_id", :column=>"environment_id"})
-> 0.0010s
rails aborted!
StandardError: An error has occurred, this and all later migrations canceled:
PG::UndefinedColumn: ERROR: column "priority" does not exist
LINE 1: ...ontent_view_environment_activation_keys" ORDER BY "priority"...
I got this when trying to run the down migration. Seems it happens at about this point. The down migration erasing the "priority" column has already run at this point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
Co-authored-by: Jeremy Lenz <[email protected]>
89677da
to
521ddfd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's take it for a real spin now!
🚀
What are the changes introduced in this pull request?
Considerations taken when implementing this change?
The approach largely mirrors what we did for content facets except 2 important changes
What are the testing steps for this pull request?
hammer activation-key create --name=foo --organization-id=1 --content-view-environments='env_label1/cv_label1,env_label2/cv_label2'
env1/cv1
andenv2/cv2
in the redhat repos file.