fix(credentials): return empty config instead of nil#607
Closed
fabianburth wants to merge 2 commits into
Closed
Conversation
Signed-off-by: Fabian Burth <fabian.burth@sap.com>
Signed-off-by: Fabian Burth <fabian.burth@sap.com>
Skarlso
previously approved these changes
Aug 22, 2025
Skarlso
reviewed
Aug 22, 2025
| func Merge(configs ...*Config) *Config { | ||
| if len(configs) == 0 { | ||
| return nil | ||
| return &Config{} |
Contributor
There was a problem hiding this comment.
Just saying that this means that we will have to check in a different way that things aren't set.
Member
There was a problem hiding this comment.
what are you referring to?
Contributor
Author
There was a problem hiding this comment.
Yeah, I'm not completely satisfied either, I'll go ahead and check again where the nil pointer was.
Contributor
There was a problem hiding this comment.
what are you referring to?
That now you can't do == nil you have to figure out what the zero value is for the struct.
Contributor
Author
There was a problem hiding this comment.
It's the ToGraph logic in `setupCredentialGraph that is not able to deal with the nil pointer.
var credCfg *credentialsRuntime.Config
var err error
if cfg := ocmctx.FromContext(cmd.Context()).Configuration(); cfg == nil {
slog.WarnContext(cmd.Context(), "could not get configuration to initialize credential graph")
// PR-COMMENT: we could move this to after the if
credCfg = &credentialsRuntime.Config{}
} else if credCfg, err = credentialsConfig.LookupCredentialConfiguration(cfg); err != nil {
return fmt.Errorf("could not get credential configuration: %w", err)
}
// PR-COMMENT: this would fix it
// if credCfg == nil {
// credCfg = &credentialsRuntime.Config{}
// }
graph, err := credentials.ToGraph(cmd.Context(), credCfg, opts)
if err != nil {
return fmt.Errorf("could not create credential graph: %w", err)
}
Included some comments with the prefix PR-COMMENT.
I'd say this is a valid fix too. Since @Skarlso comment is valid, I'd vote for closing this PR using this to fix it.
favoring an alternative suggestion
Member
|
closing as discussed |
jakobmoellerdev
added a commit
that referenced
this pull request
Aug 22, 2025
<!-- markdownlint-disable MD041 --> #### What this PR does / why we need it Align the merge logic with the change proposed in: #607 #### Which issue(s) this PR fixes <!-- Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`. --> Signed-off-by: Fabian Burth <fabian.burth@sap.com> Co-authored-by: Jakob Möller <jakob.moeller@sap.com>
jakobmoellerdev
pushed a commit
that referenced
this pull request
Aug 27, 2025
… merge logic (#608)" (#705) This reverts commit eb5a51d. <!-- markdownlint-disable MD041 --> #### What this PR does / why we need it We decided against doing the change in the credential merge logic (#607 (comment)) and only accidentally merged this PR. #### Which issue(s) this PR fixes <!-- Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`. --> Signed-off-by: Fabian Burth <fabian.burth@sap.com>
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
3 participants
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.
What this PR does / why we need it
Can't really remember where, but pretty sure we ran into a nil pointer somewhere during implementation efforts of
get cv --recursiveand agreed that this would be the best way to fix it. (:Which issue(s) this PR fixes
Contributes to: open-component-model/ocm-project#563