Only check for secret groups during registration context #2381
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
Signed-off-by: Thomas J. Fan <[email protected]>
Signed-off-by: Thomas J. Fan <[email protected]>
Signed-off-by: Thomas J. Fan <[email protected]>
thomasjpfan
requested review from
wild-endeavor,
kumare3,
eapolinario,
pingsutw,
cosmicBboy and
samhita-alla
as code owners
pmahindrakar-oss
previously approved these changes
Apr 25, 2024
| if get_plugin().secret_requires_group() and self.group is None: | ||
| # Only check for the groups during registration. | ||
| execution = FlyteContextManager.current_context().execution_state | ||
| in_registration_context = execution.mode is None |
There was a problem hiding this comment.
Should we have this to be until function which can be used at other places if needed to determine if we are in registration context
There was a problem hiding this comment.
If this comes up more often, I would add a new is_registration method to ExecutionState:
flytekit/flytekit/core/context_manager.py
Line 453 in afa1fc7
As for this PR, I rather not add public API for now. This is first time in the codebase, we check for execution.mode is None.
…ts_groups_not_required Signed-off-by: Thomas J. Fan <[email protected]>
Signed-off-by: Thomas J. Fan <[email protected]>
eapolinario
approved these changes
Apr 26, 2024
austin362667
pushed a commit
to austin362667/flytekit
that referenced
this pull request
May 9, 2024
* Only check for secret groups during registration context Signed-off-by: Thomas J. Fan <[email protected]> * Only check for secrets during registration Signed-off-by: Thomas J. Fan <[email protected]> * Adds test to check behavior Signed-off-by: Thomas J. Fan <[email protected]> * Lint Signed-off-by: Thomas J. Fan <[email protected]> --------- Signed-off-by: Thomas J. Fan <[email protected]>
austin362667
pushed a commit
to austin362667/flytekit
that referenced
this pull request
May 10, 2024
* Only check for secret groups during registration context Signed-off-by: Thomas J. Fan <[email protected]> * Only check for secrets during registration Signed-off-by: Thomas J. Fan <[email protected]> * Adds test to check behavior Signed-off-by: Thomas J. Fan <[email protected]> * Lint Signed-off-by: Thomas J. Fan <[email protected]> --------- Signed-off-by: Thomas J. Fan <[email protected]>
fiedlerNr9
pushed a commit
that referenced
this pull request
Jul 25, 2024
* Only check for secret groups during registration context Signed-off-by: Thomas J. Fan <[email protected]> * Only check for secrets during registration Signed-off-by: Thomas J. Fan <[email protected]> * Adds test to check behavior Signed-off-by: Thomas J. Fan <[email protected]> * Lint Signed-off-by: Thomas J. Fan <[email protected]> --------- Signed-off-by: Thomas J. Fan <[email protected]> Signed-off-by: Jan Fiedler <[email protected]>
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.
Tracking issue
Follow up to #2355
Why are the changes needed?
This is required to make sure that the secrets groups check never happens during execution time.
What changes were proposed in this pull request?
With this PR, the secrets group check only happens during registration time.
How was this patch tested?
There are current tests that checks
Secretsconstruction whenExecution.modeisNone. I added more tests to make sure that the groups are not checked during execution time.