Don't Unmarshal edpm_services_override #1731
Don't Unmarshal edpm_services_override #1731slagle wants to merge 1 commit intoopenstack-k8s-operators:mainfrom
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: slagle The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
bshephar
left a comment
bshephar
left a comment
There was a problem hiding this comment.
Hey man,
I couldn't open the Jira to see the full context of what the error was. But my suggestion is an implementation detail rather than a fixing the reported problem suggestion.
|
|
||
| if len(deployment.Spec.ServicesOverride) > 0 { | ||
| a.ExtraVars["edpm_services_override"] = json.RawMessage([]byte(fmt.Sprintf("\"%s\"", deployment.Spec.ServicesOverride))) | ||
| extraVarsJSON, _ := json.Marshal(deployment.Spec.ServicesOverride) |
There was a problem hiding this comment.
I feel like this err should probably be checked, no?
I understand that is complicated by the fact that this function doesn't return anything and changing the signature of the function isn't nice. It is only used in one place though, so probably not the end of the world to update the signature to:
func (a *EEJob) FormatAEEExtraVars(
aeeSpec *dataplanev1.AnsibleEESpec,
service *dataplanev1.OpenStackDataPlaneService,
deployment *dataplanev1.OpenStackDataPlaneDeployment,
nodeSet client.Object,
) error {
}
Which does also necessitate changing the signature of BuildAeeJobSpec to also return an error to facilitate back propagating the error. But I think it would probably be nicer if the error was captured and returned rather than silently dropped.
There was a problem hiding this comment.
My thinking in just dropping it is that the field value has already been validated as part of the Spec. It should marshal back to JSON just fine (in theory of course). Given we drop possible errors all over the place with "_", it felt ok to use that pattern again here.
There was a problem hiding this comment.
This is true, it wouldn't have been accepted and landed in etcd if it wasn't able to be marshalled via the API server. I guess you're right, it should probably be fine provided there's no additional mutations happening at runtime.
|
@bshephar: changing LGTM is restricted to collaborators DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
slagle
force-pushed
the
edpm_services_override
branch
from
7c6ab83 to
591a484
Compare
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/24dbbf32511c44aea579605a3a25aaf4 ✔️ openstack-k8s-operators-content-provider SUCCESS in 2h 15m 55s |
|
tempest: Unrelated by the looks of it.. |
slagle
force-pushed
the
edpm_services_override
branch
2 times, most recently
from
d90cf64 to
3b9e417
Compare
slagle
changed the title
slagle
force-pushed
the
edpm_services_override
branch
from
3b9e417 to
8cc52ca
Compare
|
/retest |
edpm_services_override is already marshal'd to JSON in ansible_execution.go, so there's no need to unmarshal and print it to string, b/c go drops the commas from the marshal'd value. This was breaking the download-cache service that relies on the proper value of edpm_services_override when using download-cache and servicesOverride on a Deployment. Jira: OSPRH-21737 Signed-off-by: James Slagle <jslagle@redhat.com>
slagle
force-pushed
the
edpm_services_override
branch
from
8cc52ca to
76b0085
Compare
| // unmarshal nested data structures | ||
| for _, variable := range keys { | ||
| if variable == "edpm_services_override" { | ||
| parsedExtraVars += fmt.Sprintf("%s: %s\n", variable, a.ExtraVars[variable]) |
There was a problem hiding this comment.
Maybe we can use yaml.marshal() to avoid the issue, but if we think that's expensive we can atleast avoid the hardcoded key check?
if len(a.ExtraVars) > 0 {
extraVarsMap := make(map[string]interface{})
for variable, rawValue := range a.ExtraVars {
var tmp interface{}
err := yaml.Unmarshal(rawValue, &tmp)
if err != nil {
return nil, err
}
extraVarsMap[variable] = tmp
}
yamlBytes, err := yaml.Marshal(extraVarsMap)
if err != nil {
return nil, err
}
setRunnerEnvVar(h, "RUNNER_EXTRA_VARS", string(yamlBytes), "extraVars", job, hashes)
}
or
for _, variable := range keys {
var tmp interface{}
err := yaml.Unmarshal(a.ExtraVars[variable], &tmp)
if err != nil {
return nil, err
}
if _, ok := tmp.([]interface{}); ok {
parsedExtraVars += fmt.Sprintf("%s: %s\n", variable, a.ExtraVars[variable])
} else {
parsedExtraVars += fmt.Sprintf("%s: %s\n", variable, tmp)
}
}
3 participants
go was formatting the servicesOverride slice without commas between the
items, which causes the value to be interpreted as a YAML list of one
string item. Marshal'ing the value to JSON first causes proper JSON
syntax so that the resulting YAML value is properly formatted with
commas.
This was breaking the download-cache service that relies on the proper
value of edpm_services_override when using download-cache and
servicesOverride on a Deployment.
Jira: OSPRH-21737
Signed-off-by: James Slagle jslagle@redhat.com