search

kubernetes-sigs / kustomize

Customization of kubernetes YAML configurations
Apache License 2.0
10.99k stars 2.25k forks source link

Semver parsing does not terminate CRLF making it returns v0.0.0 when parsing #5526

Open antoooks opened 8 months ago

antoooks commented 8 months ago

What happened?

internal libary cmd/gorepomod/internal/semver/semver.go has a function Parse(raw string) which translate a raw string into Semver object. However, if we supply string with CRLF (\r\n), it will return a default behavior zero which translates to v0.0.0. This makes semver.Parse() function cannot translate kustomize libarary current versioning e.g. kyaml right now is v0.15.1 due to Git commands returns results with CRLF.

Note: the function will not throw an error, but it will throw a default behavior

What did you expect to happen?

The correct behavior

semver.Parse("v0.15.1") will result &Semver{major: 0, minor: 15, patch: 1}

How can we reproduce it (as minimally and precisely as possible)?

Usage:

// Call a git function
currentBranchName, err := gr.run(noHarmDone, "rev-parse", "--abbrev-ref", "release-kyaml/v0.16.3")

// Split branch name string
currentVersionString := strings.Split(currentBranchName, "/")

// Supply raw string to parse function
version, _ := semver.Parse(currentVersionString[1])

Expected output

version, _ := semver.Parse("v0.16.3")
// will return v0.16.3 Semver object
&Semver{major: 0, minor: 16, patch: 3}

Actual output

// return v0.0.0 Semver object
&Semver{major: 0, minor: 0, patch: 0}

Kustomize version

v5.3.0

Operating system

Linux

k8s-ci-robot commented 8 months ago

This issue is currently awaiting triage.

SIG CLI takes a lead on issue triage for this repo, but any Kubernetes member can accept issues by applying the triage/accepted label.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
ephesused commented 8 months ago

I'm not seeing the same behavior that you're seeing. If it were failing the way you've described, then I would expect to see failures in semver_test.go, particularly TestParse/two.

I tried adding this testcase into semver_test.go, and it came back clean:

func Test5526(t *testing.T) {
    expected := SemVer{major: 0, minor: 15, patch: 1}
    actual, err := Parse("v0.15.1")
    if err != nil {
        t.Errorf("Unexpected error: '%s'", err)
    }
    if !actual.Equals(expected) {
        t.Errorf("Not equal: '%s' and '%s'", actual, expected)
    }
    if actual.String() != "v0.15.1" {
        t.Errorf("String for '%s' not as expected", actual)
    }
}

Will you detail further how to repeat the failure? Thanks.

koba1t commented 8 months ago

Hi @antoooks Can you investigate and fix this problem?

antoooks commented 8 months ago

Hi @ephesused thanks for pointing that out. The issue happens when I used .Parse() after I supplied version string I got from using git function currentBranchName, err := gr.run(noHarmDone, "rev-parse", "--abbrev-ref", "release-kyaml/v0.16.3")

I split the branch name string to extract the version number currentVersionString := strings.Split(currentBranchName, "/").

After that I supplied the currentVersionString semver.Parse() function, it returns v0.0.0. I have investigate further and finds out that the version string returned by this function call currentVersionString := strings.Split(currentBranchName, "/") has a trailing CRLF \r\n, so semver.Parse() function need to clean this before parsing the string. I will add the functionality together with CI automation on this PR #5520

Will change the title and description accordingly

k8s-triage-robot commented 5 months ago

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

You can:

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

antoooks commented 5 months ago

/remove-lifecycle stale

ephesused commented 4 months ago

Looking at this one again, I would recommend that the caller trim its whitespace prior to invoking .Parse(). Whitespace is not allowed in a semver, so I don't think semver.Parse() should allow whitespace either.

I see different behavior than you do when I try to parse v0.15.1\r\n. We both get the zero value, but where you don't get an error, I do:

strconv.Atoi: parsing "1\r\n": invalid syntax

I added this locally to TestParse and it runs clean:

    "trailing whitespace": {
        raw:    "v0.15.1\r\n",
        v:      zero,
        errMsg: "strconv.Atoi: parsing \"1\\r\\n\": invalid syntax",
    },

That fits what I would expect. Based on what I'm seeing, I believe .Parse() is acting properly.

If .Parse() isn't giving you an error in this scenario, I think the better route is to figure out what the difference is in our scenarios, and fix things so that it throws the error properly for you.

antoooks commented 4 months ago

Looking at this one again, I would recommend that the caller trim its whitespace prior to invoking .Parse(). Whitespace is not allowed in a semver, so I don't think semver.Parse() should allow whitespace either.

I see different behavior than you do when I try to parse v0.15.1\r\n. We both get the zero value, but where you don't get an error, I do:

strconv.Atoi: parsing "1\r\n": invalid syntax

I added this locally to TestParse and it runs clean:

  "trailing whitespace": {
      raw:    "v0.15.1\r\n",
      v:      zero,
      errMsg: "strconv.Atoi: parsing \"1\\r\\n\": invalid syntax",
  },

That fits what I would expect. Based on what I'm seeing, I believe .Parse() is acting properly.

If .Parse() isn't giving you an error in this scenario, I think the better route is to figure out what the difference is in our scenarios, and fix things so that it throws the error properly for you.

Thanks for pointing it out, I have included trim inside Parse() on my PR (#5449) because I need it for the feature I'm working on. I see your point and will include such testing on my changes as well

antoooks commented 4 months ago

Hi @ephesused , I am able to reproduce the condition. So you cannot use "v0.15.1\r\n" because that is a string and strconv.Atoi() will read \r and \n literally, as runes, though it will trigger the error handling condition anyway, it does not represent this issue. So to reproduce correctly you have to use fmt.Sprintf("v0.15.1\r\n"), anyways I will use "v0.15.1\r\n" since it's a better practice according to the linter.

And secondly, throwing error means it works as expected. It's just that the test does not consume the return value and only look for the error that is being thrown together with zero. It is the same case as v1.x.222 test case (another non-digit test case), only difference being the non-digit for CRLF case is /r/n and is placed at the last digit.

On the source code you can see this conditional:

// kustomize/cmd/gorepomod/internal/semver.go, L55-58

      n[i], err = strconv.Atoi(fields[i])
      if err != nil {
          return zero, err
      }

Which makes it supposedly throw zero or v0.0.0 upon non-digit input.

ephesused commented 4 months ago

Sorry, I don't understand what you're describing. If you have a test case that shows the incorrect semver.Parse() behavior, will you please copy-and-paste it in an update to this ticket?

From what I see, semver.Parse() is behaving properly, and adhering to the semantic versioning standard. That is why I don't think it should change. Instead, I believe the caller should trim whitespace prior to calling semver.Parse().

k8s-triage-robot commented 1 month ago

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

You can:

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot commented 3 weeks ago

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

You can:

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten