Skip to content

fix: improve wording for unassignable Policy #4414

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

romanblanco
Copy link
Member

When Compliance tries to assign Host to a Policy, it returns either 202 in case of success or 404 in any other case.

When one tries to assign a Host that already has a Policy assigned, the Policy is not anymore in the list of available options, hence the process can not succeed with creating the link and returns 404.

That in the logic of insights-client seems as failure, and it returns non-zero error status with the confusing message "Policy ... does not exist".

This commit is to improve the message sent in such case to make it less misleading.

    When Compliance tries to assign Host to a Policy,
    it returns either 202 in case of success or 404 in any other case.

    When one tries to assign a Host that already has a Policy assigned,
    the Policy is not anymore in the list of available options,
    hence the process can not succeed with creating the link and returns 404.

    That in the logic of insights-client seems as failure, and it returns
    non-zero error status with the confusing message
    "Policy ... does not exist".

    This commit is to improve the message sent in such case to make it
    less misleading.
@LightOfHeaven1994
Copy link
Contributor

@romanblanco as there are might be 2 cases:

  1. Policy doesn't exist
  2. Policy exists but host can't be assigned due to already existing association

For the 2nd case we might want to add additional request to else block: systems/{ID}/policies?limit=100&ids_only=true and check if policy id is already there - then we return message "Policy already associated to host"
And if not - "Policy doesn't exist"

It's not cool to have another request for that but it will be beneficial for all of us + this request is easy one for backend.

You also need to sign off your PR (git commit --amend -s) and update tests

@@ -227,7 +227,7 @@ def policy_link(self, policy_id, opt):
logger.info("Operation completed successfully.\n")
Copy link
Contributor

@Mishrasubha Mishrasubha Apr 3, 2025

Choose a reason for hiding this comment

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

can we also change Operation completed successfully to something more specific like System successfully assigned to policy?

@xiangce
Copy link
Contributor

xiangce commented Apr 7, 2025

@romanblanco, Please also update the expected assert string in tests. Thanks.

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.

4 participants