(fix) : Fix check to verify label versions when it is =v4.x #148
Conversation
pkg/validation/internal/testdata/dockerfile_v4_8/bundle_without_label.Dockerfile
Outdated
Show resolved
Hide resolved
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: camilamacedo86, njhale The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
pkg/validation/internal/community.go
Outdated
| doubleCote := "\"" | ||
| singleCote := "'" | ||
| value = strings.ReplaceAll(value, singleCote, "") | ||
| value = strings.ReplaceAll(value, doubleCote, "") | ||
| value = strings.ReplaceAll(value, "v", "") | ||
| return value |
There was a problem hiding this comment.
| doubleCote := "\"" | |
| singleCote := "'" | |
| value = strings.ReplaceAll(value, singleCote, "") | |
| value = strings.ReplaceAll(value, doubleCote, "") | |
| value = strings.ReplaceAll(value, "v", "") | |
| return value | |
| value, err := strconv.Unquote(value) | |
| if err != nil { | |
| return "", err | |
| } | |
| return strings.TrimPrefix(value, "v"), nil |
A couple of notes:
strconv.Unquote()is a safe unquote that tells you via an error if the string was not correctly quoted- It seems like
strings.TrimPrefix(value, "v")is safer than just replacing all "v"s in a string. Otherwise something like this would be valid"vvv4vv.vvv8vvv".
If it helps, I wrote this function last week to convert from the label format to a semver range and then do a comparison: https://github.com/joelanford/community-operators-prod/blob/d10cb2dd9afda91a0626c10be93d1b819587d4f2/builder/main.go#L341-L392
There was a problem hiding this comment.
Hi @joelanford,
Not proud of this code at all. We need to clean up it and we could centralize something as yoursexample.
Your suggestion makes the code fails with invalid syntax. So, can we do that in future follow up?
There was a problem hiding this comment.
Is my example failing just because it introduces a need to have the function signature return (string, error)?
Either way, I think we should find a way to use strconv.Unquote and strings.TrimPrefix(value, "v") so that we don't accidentally allow values we would consider to be invalid.
Any time we are permissive with a value, we must always be permissive from that point forward, which is a bummer. I encountered lots of cases like this implementing file-based configs (e.g. icons that don't match their mediatypes or that don't even have valid base64 data).
There was a problem hiding this comment.
Is my example failing just because it introduces a need to have the function signature return (string, error)?
No, of course not. Note that strconv.Unquote is what is returning syntax error.
Note as well that we can have more than only v4.8. It can be for example v4.8-v4.9.
We need cleanups and other improvements by sure. I am working now on follow-ups.
There was a problem hiding this comment.
Hi @joe,
You can easily check here that the suggestion does not work well with one of the valid options: https://play.golang.org/p/mdKQuyULHdq.
Also, your code implemented and shared is nice but it seems to fail with v4.8-v4.9 which is a valid scenario: https://play.golang.org/p/bNgxNTWRc-i . I fixed your code (https://play.golang.org/p/cCF-EajZzRn) and I applied that in its follows up. Please, check its follow up with your code suggestion: #149.
|
/lgtm |
Description
CVP reported the error in the check when the label is = v4.8.
This PR fixes the error and add a test to cover this scenario.