Add offline token argument to inventory command #45
+12
−4
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
This PR modifies the inventory CLI command to accept an offline_token argument. This should resolve an issue with generating an access token that I'm encountering with a CI pipeline. With this change, the job can pass an environment variable containing the offline token to the Manifester CLI, which mirrors how we access the same token in another CI use case.
JacobCallahan
approved these changes
Jul 30, 2024
ogajduse
reviewed
Jul 31, 2024
manifester/manifester.py
Outdated
Comment on lines
36
to
39
| if kwargs.get("offline_token") is not None: | ||
| self.offline_token = kwargs.get("offline_token") | ||
| else: | ||
| self.offline_token = settings.get("offline_token") |
There was a problem hiding this comment.
Currently, with validators disabled, we might bump into self.offline_token being none if none of kwargs and settings contain the offline token. Should we raise an exception in that case?
Suggested change
| if kwargs.get("offline_token") is not None: | |
| self.offline_token = kwargs.get("offline_token") | |
| else: | |
| self.offline_token = settings.get("offline_token") | |
| if not self.offline_token := kwargs.get("offline_token", settings.get("offline_token")): | |
| raise Exception("Offline token was not set") # to be replaced with less general exception type |
There was a problem hiding this comment.
Ack, good idea. I will make this change.
There was a problem hiding this comment.
@ogajduse It turns out that := can't be used to set instance attributes. Doing so throws SyntaxError: cannot use assignment expressions with attribute. But I've added another condition to catch this. I also added an exception that prints the error text when generating an access token fails, which should provide more visibility into failures at that stage.
synkd
force-pushed
the
accept_offline_token_arg_for_inventory_command
branch
from
4afd1ae to
94a9f71
Compare
synkd
added a commit
to synkd/manifester
that referenced
this pull request
Sep 11, 2024
This PR makes a similar change to the change made in SatelliteQE#45 but for the `manifester delete` subcommand. This is to support a CI usecase of the manifester CLI. The PR also reformats the `commands.py` module with Ruff.
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.
This PR modifies the inventory CLI command to accept an offline_token argument. This should resolve an issue with generating an access token that I'm encountering with a CI pipeline. With this change, the job can pass an environment variable containing the offline token to the Manifester CLI, which mirrors how we access the same token in another CI use case.