Refactor settings and reformat multiple files #8
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously, API URLs were hardcoded in the body of manifester/manifester.py, and the offline token (which is used to generate an access token, which is in turn used to authenticate API calls) was a top-level setting. This commit makes the offline_token setting a child setting of each manifest_category, and it moves the API URLs to the settings file, also as children of each manifest_category. This should enable greater flexibility by letting users associate manifests with multiple RHSM accounts (potentially in multiple environments, such as Red Hat's staging environment) in a single settings file. This commit also makes numerous formatting changes as a result of running pre-commit. Additionally, the offline_token validator in manifester/settings.py has been commented out as it fails to validate with the offline_token setting nested under the manifest categories.
ogajduse
reviewed
Jul 28, 2022
| subscription_data: | ||
| # name should be an exact match of the subscription name as listed on the Customer Portal | ||
| - name: "Software Collections and Developer Toolset" | ||
| quantity: 1 | ||
| - name: "Red Hat Ansible Automation Platform, Standard (100 Managed Nodes)" | ||
| quantity: 1 | ||
| simple_content_access: "enabled" | ||
| url: | ||
| token_request: "https://sso.redhat.com/auth/realms/redhat-external/protocol/openid-connect/token" | ||
| allocations: "https://api.access.redhat.com/management/v1/allocations" |
There was a problem hiding this comment.
Nice! People can simply generate manifest on stage using manifester!
Edit: a few minutes later, I read the PR description.
ogajduse
reviewed
Jul 28, 2022
mshriver
reviewed
Jul 28, 2022
| self.subscription_data = manifest_data.subscription_data | ||
| self.sat_version = kwargs.get("sat_version", manifest_data.sat_version) | ||
| self.allocation_name = allocation_name or "".join( | ||
| random.sample(string.ascii_letters, 10) |
There was a problem hiding this comment.
Consider adding a fauxfactory dependency for this.
There was a problem hiding this comment.
right now it is just a single call, but if more random data is needed later, that is a possibility. or the more performant
str(uuid4()).split("-")[-1]
JacobCallahan
requested changes
Jul 28, 2022
manifester/manifester.py
Outdated
Comment on lines
19
to
21
| self.offline_token = kwargs.get( | ||
| "offline_token", self.manifest_data.offline_token | ||
| ) |
There was a problem hiding this comment.
I think it would be a good idea to modify this so the offline token could be set at the top level or overridden at the manifest level. That way someone using the same token for each doesn't need to duplicate the setting for each manifest.
Suggested change
| self.offline_token = kwargs.get( | |
| "offline_token", self.manifest_data.offline_token | |
| ) | |
| self.offline_token = kwargs.get( | |
| "offline_token", self.manifest_data.get("offline_token", settings.offline_token) | |
| ) |
manifester/manifester.py
Outdated
| self._subscription_pools=None | ||
| logger.debug( | ||
| f"Received response status {add_entitlements.status_code}." | ||
| f"Trying to find another pool." |
There was a problem hiding this comment.
Suggested change
| f"Trying to find another pool." | |
| "Trying to find another pool." |
manifester/manifester.py
Outdated
| ) | ||
| break | ||
| else: | ||
| raise Exception(f"Something went wrong while adding entitlements. Received response status {add_entitlements.status_code}.") | ||
| raise Exception( | ||
| f"Something went wrong while adding entitlements. Received response status " |
There was a problem hiding this comment.
Suggested change
| f"Something went wrong while adding entitlements. Received response status " | |
| "Something went wrong while adding entitlements. Received response status " |
Access to the internal Red Hat staging environment is mediated through a
proxy server. To support compatibility with this environment, this
commit adds a `proxies` setting to each manifest_category and adds a
`proxies` argument to each request in manifester.py. To access the
production RHSM API without a proxy, the value of the dictionary in the
`proxies` setting can be left as an empty string (i.e. `proxies:
{"https": ""}).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, API URLs were hardcoded in the body of
manifester/manifester.py, and the offline token (which is used to
generate an access token, which is in turn used to authenticate API
calls) was a top-level setting. This commit makes the offline_token
setting a child setting of each manifest_category, and it moves the API
URLs to the settings file, also as children of each manifest_category.
This should enable greater flexibility by letting users associate
manifests with multiple RHSM accounts (potentially in multiple
environments, such as Red Hat's staging environment) in a single
settings file.
This commit also makes numerous formatting changes as a result of
running pre-commit.
Additionally, the offline_token validator in manifester/settings.py has
been commented out as it fails to validate with the offline_token
setting nested under the manifest categories.