feat: match labels FromEnvironmentFieldPath#119
Open
tenstad wants to merge 6 commits into
Open
Conversation
phisco
reviewed
Jun 12, 2026
phisco
reviewed
Jun 12, 2026
| // Note(phisco): We need to compute the selectors even if we already | ||
| // requested them already at the previous iteration. | ||
| requirements, err := buildRequirements(in, oxr) | ||
| env, _ := request.GetContextKey(req, FunctionContextKeyEnvironment) |
Collaborator
There was a problem hiding this comment.
missing error handling here
Contributor
Author
There was a problem hiding this comment.
Yeah, I don't check the ok from v, ok := f[key] as there is no guarantee that the environment key exists in the context. And we want the resulting map to just be empty if it does not exist exit. Both .GetStructValue() and .AsMap() are nil-safe, so there is no need to do conditionally call them. Added a comment to clarify.
Contributor
Author
There was a problem hiding this comment.
What do you think is best? Skipping the check, or performing it as in #119 (comment)
Signed-off-by: Amund Tenstad <github@amund.io>
Signed-off-by: Amund Tenstad <github@amund.io>
9a077ed to
51fb4d5
Compare
…h typo Signed-off-by: Amund Tenstad <github@amund.io>
Signed-off-by: Amund Tenstad <github@amund.io>
7dbda1a to
508610e
Compare
Signed-off-by: Amund Tenstad <github@amund.io>
tenstad
commented
Jun 12, 2026
Comment on lines
+64
to
+66
| // Skip key presence check as the context key is not required and map conversion is nil safe. | ||
| envCtx, _ := request.GetContextKey(req, FunctionContextKeyEnvironment) | ||
| env := envCtx.GetStructValue().AsMap() |
Contributor
Author
There was a problem hiding this comment.
Could also just do the if check I guess, avoiding the comment and potential confusion.
Suggested change
| // Skip key presence check as the context key is not required and map conversion is nil safe. | |
| envCtx, _ := request.GetContextKey(req, FunctionContextKeyEnvironment) | |
| env := envCtx.GetStructValue().AsMap() | |
| var env map[string]any | |
| if envCtx, ok := request.GetContextKey(req, FunctionContextKeyEnvironment); ok { | |
| env = envCtx.GetStructValue().AsMap() | |
| } |
This file contains hidden or 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
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.
Description of your changes
It is currently difficult to lookup EnvironmentConfigs based on values from environment. For example fetching one environment config (or resource, using function-extra-resources) and then fetch another environment config based on values in the first one.
The PR extends the types of
matchLabelsto includeFromEnvironmentFieldPath.It reuses the
valueFromFieldPathandfromFieldPathPolicyfields used byFromCompositeFieldPath, as they did not contain composite, but could instead createvalueFromEnvironmentFieldPathandfromEnvironmentFieldPathPolicy.Tests are updated to also test
FromEnvironmentFieldPathwhereverFromCompositeFieldPathis tested.I have: