fix: config injection new config validation #49
Conversation
|
Github Enforcer opened Task: RK-18981 |
sonariorobot
changed the title
gosharo
changed the title
Signed-off-by: Gosha <[email protected]>
Signed-off-by: Gosha <[email protected]>
Signed-off-by: Gosha <[email protected]>
Signed-off-by: Eli Hatamov <[email protected]> Signed-off-by: Gosha <[email protected]>
gosharo
force-pushed
the
config-injection-bugfixes
branch
from
605456f to
0deec13
Compare
Signed-off-by: GoshaDozoretz <[email protected]>
Signed-off-by: Gosha <[email protected]>
Signed-off-by: Gosha <[email protected]>
Signed-off-by: Gosha <[email protected]>
Signed-off-by: Gosha <[email protected]>
Signed-off-by: Gosha <[email protected]>
Signed-off-by: Gosha <[email protected]>
Signed-off-by: Gosha <[email protected]>
| panic("implement me") | ||
| func (wfc *WorkflowsClientImpl) SelectConfig(workflowsBatch *common.WorkflowsBatch) (string, error) { | ||
| var configName string | ||
| ok := IsConfigExists(&wfc.cfg.WorkflowsConfig, "default") |
There was a problem hiding this comment.
Can be simplified to:
If config != "" and exists
Use it
Else
Use default
Default will always exists so no need to check for it
There was a problem hiding this comment.
default not always exists..
it there only if configure, if not, it will use the CRDs default values..
There was a problem hiding this comment.
So what this function returns in case there is no default and no other config ?
There was a problem hiding this comment.
Looks weird that there is a default to the default
There was a problem hiding this comment.
I would not handle config name default as special case here but as regular config that user decided to call it "default" so he needs to explicitly mention he wants to use it
There was a problem hiding this comment.
It will make the config selection logic more predictable - you get what you asked for if it's existing
There was a problem hiding this comment.
the CRD's default is blank, wheres piper default is the DevOps configuration provided
| func (wfc *WorkflowsClientImpl) SetConfig(wf *v1alpha1.Workflow, spec *v1alpha1.WorkflowSpec) error { | ||
| //TODO implement me | ||
| panic("implement me") | ||
| func (wfc *WorkflowsClientImpl) SelectConfig(workflowsBatch *common.WorkflowsBatch) (string, error) { |
There was a problem hiding this comment.
This function never returns err so you can get rid of it in the return params.
It always returns just a string
Signed-off-by: Gosha [email protected]
Pull Request
Description
Please provide a brief description of the changes made in this pull request.
Related Issue(s)
If this pull request addresses or relates to any open issues, please mention them here using the syntax
Fixes #<issue_number>orResolves #<issue_number>.Checklist
Before submitting this pull request, please ensure that you have completed the following tasks:
Testing Instructions
Please provide clear instructions on how to test and verify the changes made in this pull request.
Additional Information
Add any additional information or context that would be helpful in understanding and reviewing this pull request.