Fixes #37895 - Handle version removal for multi-CV activation key #11246

pavanshekar · 2024-12-03T21:52:11Z

What are the changes introduced in this pull request?

Added checks for multi-CV associations and helper methods to handle version removal from the environment, cleaning up relevant activation key and environment links to ensure only the targeted version is removed without affecting unrelated associations.

Considerations taken when implementing this change?

Ensured selective removal of associations to avoid reassigning multi-CV AK to single CVE and impacting unrelated environments or activation keys, preserved existing associations while removing the targeted version, reused existing methods for consistency, and added checks for edge cases like multiple activation keys linked to the same environment.

What are the testing steps for this pull request?

Create a multi-CV activation key

Promote a CV Version to multiple environments
Using Hammer CLI, update the activation key to use mutli CVE
Example command: hammer activation-key update --organization="Default Organization" --id=1 --content-view-environments="ABC/cv1,XYZ/cv1"
Confirm that the activation key uses multi CVEs

Remove the version from the environment

Go to the CV associated with the activation key -> Versions
Click on the 3 dots of the respective CV Version and select the option "Remove from environments"
Follow the steps to remove the CV Version from the environment
A task will be created which should successfully remove the version from the environment when the CVE is in use by a multi-CV activation key

jeremylenz

Thanks @pavanshekar!

The backend logic seems to work well.

Before:

$ hammer activation-key list --organization-id 1
---|---------|----------------|----------------------------|-------------------------------
ID | NAME    | HOST LIMIT     | CONTENT VIEW ENVIRONMENTS  | MULTI CONTENT VIEW ENVIRONMENT
---|---------|----------------|----------------------------|-------------------------------
1  | ak1     | 2 of Unlimited | Library                    | no                            
2  | akmulti | 0 of Unlimited | env1/cv1,env2/cv1,env3/cv1 | yes                           
---|---------|----------------|----------------------------|-------------------------------

After:

$ hammer activation-key list --organization-id 1
---|---------|----------------|---------------------------|-------------------------------
ID | NAME    | HOST LIMIT     | CONTENT VIEW ENVIRONMENTS | MULTI CONTENT VIEW ENVIRONMENT
---|---------|----------------|---------------------------|-------------------------------
1  | ak1     | 2 of Unlimited | Library                   | no                            
2  | akmulti | 0 of Unlimited | env1/cv1,env3/cv1         | yes

However, I see a couple issues.

First, the UI wizard shows the AK as needing to be reassigned, and as a user I must still select a new LCE/CV for it:

The LCE/CV I select is ignored (which is correct, since the backend logic does what we want. But still confusing for the user.)

Secondly, after I complete the wizard, the activation key's CVE priority becomes invalid:

[4] pry(main)> akmulti.content_view_environment_activation_keys.map { |cveak| [cveak.content_view_environment.label, cveak.priority] }
=> [["env1/cv1", 0], ["env3/cv1", 0]]

The priorities should be unique and sortable in a deterministic way, e.g. env1/cv1 should have priority 0 and env3/cv1 should have priority 1.

So, my suggestion for the UI is (if it's possible) - Make it so multiCV AKs don't show up as "needing to be reassigned" in the wizard .. but still do the backend logic as you have it here. This could be corrected in the frontend only.

I will comment below about the second issue.

jeremylenz · 2024-12-11T18:26:25Z

app/lib/actions/katello/content_view_environment/destroy.rb

+
+        def remove_activation_key_associations(cv_env)
+          cv_env.activation_keys.each do |key|
+            key.content_view_environment_activation_keys.find_by(content_view_environment: cv_env)&.destroy


This should use ActivationKey#content_view_environments= instead:

Suggested change

key.content_view_environment_activation_keys.find_by(content_view_environment: cv_env)&.destroy

key.content_view_environments = key.content_view_environments - cv_env

The ActivationKey model redefines the content_view_environments= method here: https://github.com/Katello/katello/blob/master/app/models/katello/activation_key.rb#L92

This ensures that the priority gets recalculated properly.

Ack... I will make the change and push it.

jeremylenz · 2024-12-11T18:32:24Z

app/lib/actions/katello/content_view_environment/destroy.rb

+          if cv_env.activation_keys.any?(&:multi_content_view_environment?)
+            remove_activation_key_associations(cv_env)
+            remove_content_view_environment_association(content_view, environment)
+            plan_self(:id => cv_env.id, :docker_cleanup => false)
+            return true
+          end


I'm questioning whether this logic is okay here within the main ContentViewEnvironment::Destroy action, or if it would be better within a new sub-action.. @sjha4 thoughts?

We can refactor it more so the logic separation is clearer to read.

jeremylenz · 2024-12-11T20:06:58Z

Make it so multiCV AKs don't show up as "needing to be reassigned" in the wizard .. but still do the backend logic as you have it here. This could be corrected in the frontend only.

To clarify a bit here:

If there are no single-env AKs needing reassignment, wizard step 2 doesn't need to be shown.
If there are multi-env AKs needing removal of a content view environment, that should be shown in the Review step as an additional message: "1 multi-environment activation key will have content view environment env1/cv1 removed"

jeremylenz · 2024-12-11T20:08:29Z

hm, we still need a way to show which activation keys will need CVEs removed. Maybe still show step 2, but don't allow the user to select a new lce/cv if it's only multi-env AKs. @sjha4 thoughts?

jeremylenz · 2024-12-12T18:21:50Z

I think we will have the same problem for reassigning hosts. If that's easy to solve here, let's do it, but otherwise that probably should be its own followup issue.

pavanshekar · 2024-12-13T14:23:27Z

@jeremylenz I have included the suggested changes in destroy.rb, added a multi-CV environment alert warning to the UI in step 2, and pushed the changes.

jeremylenz

Something seems to be broken now. It no longer removes the CVE from my multi-env AK. Instead, it reassigns it and becomes a single-env AK (which is the opposite of what the warning banner says it will do.)

It's not this change:

key.content_view_environments = key.content_view_environments - [cv_env]

since I tested that in rails console and it does work as expected. But I'm not sure what else changed..

jeremylenz · 2024-12-13T15:14:31Z

webpack/scenes/ContentViews/Details/Versions/Delete/RemoveSteps/CVReassignActivationKeysForm.js

+  useDeepCompareEffect(() => {
+    if (activationKeysResponse?.results) {
+      const hasMultiCVEnv = activationKeysResponse.results.some(key =>
+        key.multi_content_view_environment);
+      setMultiCVWarning(hasMultiCVEnv);
+    }
+  }, [activationKeysResponse]);
+


This doesn't need a useState or a useDeepCompareEffect. It still seems to work fine if we just

const multiCVWarning = activationKeysResponse?.results?.some?.(key => key.multi_content_view_environment);

Ack... Will implement the change

@jeremylenz I have incorporated the suggested changes and pushed the code.

jeremylenz

Didn't realize that your update comment wasn't just about the useDeepCompareEffect! Thanks for changing the logic like we discussed too.

On the wizard Review step, it still says that all affected keys will be reassigned. This is now not true anymore.

Should also add a second statement like

Content view environment will be removed from X multi-environment activation keys

Other than that it's looking good! Will test one more time once this is updated.

jeremylenz · 2024-12-18T21:08:37Z

webpack/scenes/ContentViews/Details/Versions/Delete/RemoveSteps/CVReassignActivationKeysForm.js

@@ -102,8 +108,21 @@ const CVReassignActivationKeysForm = () => {
    cvSelectOptions,
  });

+  const multiCVRemovalInfo = __('The current version will be removed from the multi-CV activation keys. The reassignment will not be added to these activation keys in this step. See hammer activation-key --help for more details.');


Suggested change

const multiCVRemovalInfo = __('The current version will be removed from the multi-CV activation keys. The reassignment will not be added to these activation keys in this step. See hammer activation-key --help for more details.');

const multiCVRemovalInfo = __('This environment is used in one or more multi-environment activation keys. The environment will simply be removed from the multi-environment keys. The content view and lifecycle environment you select here will only apply to single-environment activation keys. See hammer activation-key --help for more details.');

Ack... I have incorporated the suggested changes and pushed the updated code.

jeremylenz · 2024-12-19T15:24:58Z

Angular failures are unrelated (no idea what's going on there), but React tests look like they need some love :)

pavanshekar · 2024-12-19T15:31:28Z

Yes. 1 of the test suite was failing because I updated the code for calculating the count to display appropriate warnings for single and multi-cv AKs. I have resolved it and pushed the changes.

jeremylenz

Works as expected.

Thanks @pavanshekar! ACK 👍

pr-processor bot added the Not yet reviewed label Dec 3, 2024

pavanshekar marked this pull request as ready for review December 4, 2024 17:01

jeremylenz reviewed Dec 11, 2024

View reviewed changes

pr-processor bot removed the Not yet reviewed label Dec 11, 2024

jeremylenz reviewed Dec 11, 2024

View reviewed changes

pavanshekar force-pushed the issue-37895 branch from 705ba6a to eed4a44 Compare December 13, 2024 14:22

jeremylenz reviewed Dec 13, 2024

View reviewed changes

pavanshekar force-pushed the issue-37895 branch from eed4a44 to 9d489fa Compare December 16, 2024 21:04

jeremylenz reviewed Dec 18, 2024

View reviewed changes

pavanshekar force-pushed the issue-37895 branch 3 times, most recently from b504146 to f948b91 Compare December 19, 2024 13:51

Fixes #37895 - Handle version removal for multi-CV activation key

9e96d3d

pavanshekar force-pushed the issue-37895 branch from f948b91 to 9e96d3d Compare December 19, 2024 15:30

jeremylenz approved these changes Dec 19, 2024

View reviewed changes

jeremylenz merged commit 380e7ed into Katello:master Dec 19, 2024
24 of 26 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #37895 - Handle version removal for multi-CV activation key #11246

Fixes #37895 - Handle version removal for multi-CV activation key #11246

pavanshekar commented Dec 3, 2024 •

edited

Loading

jeremylenz left a comment

jeremylenz Dec 11, 2024

pavanshekar Dec 12, 2024

jeremylenz Dec 11, 2024

sjha4 Dec 12, 2024

jeremylenz commented Dec 11, 2024

jeremylenz commented Dec 11, 2024

jeremylenz commented Dec 12, 2024

pavanshekar commented Dec 13, 2024

jeremylenz left a comment

jeremylenz Dec 13, 2024

pavanshekar Dec 13, 2024

pavanshekar Dec 16, 2024

jeremylenz left a comment

jeremylenz Dec 18, 2024

pavanshekar Dec 19, 2024

jeremylenz commented Dec 19, 2024

pavanshekar commented Dec 19, 2024

jeremylenz left a comment

	key.content_view_environment_activation_keys.find_by(content_view_environment: cv_env)&.destroy
	key.content_view_environments = key.content_view_environments - cv_env

	const multiCVRemovalInfo = __('The current version will be removed from the multi-CV activation keys. The reassignment will not be added to these activation keys in this step. See hammer activation-key --help for more details.');
	const multiCVRemovalInfo = __('This environment is used in one or more multi-environment activation keys. The environment will simply be removed from the multi-environment keys. The content view and lifecycle environment you select here will only apply to single-environment activation keys. See hammer activation-key --help for more details.');

Fixes #37895 - Handle version removal for multi-CV activation key #11246

Fixes #37895 - Handle version removal for multi-CV activation key #11246

Conversation

pavanshekar commented Dec 3, 2024 • edited Loading

What are the changes introduced in this pull request?

Considerations taken when implementing this change?

What are the testing steps for this pull request?

jeremylenz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremylenz commented Dec 11, 2024

jeremylenz commented Dec 11, 2024

jeremylenz commented Dec 12, 2024

pavanshekar commented Dec 13, 2024

jeremylenz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremylenz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremylenz commented Dec 19, 2024

pavanshekar commented Dec 19, 2024

jeremylenz left a comment

Choose a reason for hiding this comment

pavanshekar commented Dec 3, 2024 •

edited

Loading