Conversation
5a1f175 to
7400d1d
Compare
liranmauda
left a comment
There was a problem hiding this comment.
LGTM
@dannyzaken WDYT?
4179fa8 to
2fa4c94
Compare
2fa4c94 to
a7e22b6
Compare
pkg/system/phase2_creating.go
Outdated
| annotationValue, annotationExists := util.GetAnnotationValue(r.NooBaa.Annotations, "MulticloudObjectGatewayProviderMode") | ||
| annotationBoolVal := false | ||
| if annotationExists { | ||
| annotationBoolVal = strings.ToLower(annotationValue) == strconv.FormatBool(true) |
There was a problem hiding this comment.
Why is strconv.FormatBool necessary?
There was a problem hiding this comment.
to prevent typos at the very least, i try to avoid hardcoding text strings if not necessary
There was a problem hiding this comment.
There is no need to pass the env through the noobaa-config config map. the purpose of this configmap is to provide configurable envs that can be modified by the user. This env is controlled by the annotation and is not meant to be configured by the user.
I suggest adding this env to the setDesiredCoreEnv function. add a case for this env that will set the value according to the annotation
42ceadb to
88cefae
Compare
88cefae to
12186c5
Compare
12186c5 to
4134a70
Compare
dannyzaken
left a comment
There was a problem hiding this comment.
one small comment. other than that looks good
pkg/system/phase2_creating.go
Outdated
| annotationValue, annotationExists := util.GetAnnotationValue(r.NooBaa.Annotations, "MulticloudObjectGatewayProviderMode") | ||
| annotationBoolVal := false | ||
| if annotationExists { | ||
| annotationBoolVal = strings.ToLower(annotationValue) == strconv.FormatBool(true) |
There was a problem hiding this comment.
The strconv is not so readable. Please use a const instead. You can define trueStr or a similar const in the file scope. Use it also for setting the env value below
62df992 to
5f76184
Compare
| if annotationBoolVal { | ||
| c.Env[j].Value = "true" | ||
| } |
There was a problem hiding this comment.
| if annotationBoolVal { | |
| c.Env[j].Value = "true" | |
| } | |
| if annotationBoolVal { | |
| c.Env[j].Value = trueStr | |
| } else { | |
| c.Env[j].Value = falseStr | |
| } |
| configMapKeyRef: | ||
| name: noobaa-config | ||
| key: NOOBAA_LOG_LEVEL | ||
| - name: CONFIG_JS_RESTRICT_RESOURCE_DELETION |
There was a problem hiding this comment.
An env variable should not start with CONFIG_JS_
| - name: CONFIG_JS_RESTRICT_RESOURCE_DELETION | |
| - name: RESTRICT_RESOURCE_DELETION |
pkg/system/phase2_creating.go
Outdated
| } else { | ||
| c.Env[j].Value = "" | ||
| } | ||
| case "CONFIG_JS_RESTRICT_RESOURCE_DELETION": |
There was a problem hiding this comment.
| case "CONFIG_JS_RESTRICT_RESOURCE_DELETION": | |
| case "RESTRICT_RESOURCE_DELETION": |
5f76184 to
4b90303
Compare
| name: noobaa-config | ||
| key: NOOBAA_LOG_LEVEL | ||
| - name: RESTRICT_RESOURCE_DELETION | ||
| value: "false" |
There was a problem hiding this comment.
We do not need a default.
anything other than true will result in false.
With that said, we can keep it to make it more explicit.
@dannyzaken WDYT?
There was a problem hiding this comment.
@liranmauda Can you please approve the PR so this could merge
5ccdd5d to
4b90303
Compare
4b90303 to
c1d5ba3
Compare
moved check location and and set env var to optional Signed-off-by: Amit Berner <aberner@redhat.com>
c1d5ba3 to
b014677
Compare
Explain the changes
Issues: Fixed #xxx / Gap #xxx
Testing Instructions: