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 #35961 - Simple Content Access API shouldn't check upstream connection #10422

Merged
merged 1 commit into from
Jan 23, 2023

Conversation

jeremylenz
Copy link
Member

What are the changes introduced in this pull request?

The organizations/:id/simple_content_access/ API endpoints were never updated after the upgrade to Candlepin 4.2. This means that

  • you could not enable or disable SCA via the API
  • you could not check SCA eligibility via the API

This change reflects that SCA is now tied to the organization, and no longer requires a connection to the upstream Candlepin consumer.

Considerations taken when implementing this change?

I'm not entirely sure if the /eligible endpoint is necessary any more. But I left it in for backward compatibility.

What are the testing steps for this pull request?

Get an organization with no manifest imported

curl -sku admin:changeme -H "content-type: application/json" -X PUT <server url>/katello/api/organizations/1/simple_content_access/disable
curl -sku admin:changeme -H "content-type: application/json" -X PUT <server url>/katello/api/organizations/1/simple_content_access/enable
curl -sku admin:changeme -H "content-type: application/json" -X GET <server url>/katello/api/organizations/1/simple_content_access/eligible

Before:

{"displayMessage":"Current organization does not have a manifest imported.","errors":["Current organization does not have a manifest imported."]}

After:

API responses as expected

@theforeman-bot
Copy link

Issues: #35961

@jeremylenz
Copy link
Member Author

Fixed tests, ready for review ✔️

@chris1984 chris1984 self-assigned this Jan 19, 2023
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.

Code looks sane, left one comment. Will start to test

@jeremylenz
Copy link
Member Author

@chris1984 Added deprecation warning

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.

ACK, works great. Thanks for adding the dep warning @jeremylenz

@jeremylenz jeremylenz merged commit 186a503 into Katello:master Jan 23, 2023
chris1984 pushed a commit that referenced this pull request Feb 8, 2023
chris1984 pushed a commit that referenced this pull request Feb 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants