search

kubernetes-sigs / zeitgeist

Zeitgeist: the language-agnostic dependency checker
https://godoc.org/sigs.k8s.io/zeitgeist
Apache License 2.0
181 stars 23 forks source link

semver.Parse vs semver.ParseTolerant #859

Closed chlunde closed 1 week ago

chlunde commented 6 months ago

What happened:

Our zeitgeist validate workflow randomly stopped today with the error:

level=fatal msg="error during command execution: &fmt.wrapError{msg:\"checking remote dependencies: No Major.Minor.Patch elements found\", err:(*errors.errorString)(0xc000e16010)}"
level=fatal msg="error during command execution: &errors.errorString{s:\"No Major.Minor.Patch elements found\"}"

What you expected to happen:

  1. Include context (dependency name) in the error:
diff --git a/dependency/dependency.go b/dependency/dependency.go
index d6a3eaa..a17c001 100644
--- a/dependency/dependency.go
+++ b/dependency/dependency.go
@@ -527,12 +527,12 @@ func (c *Client) checkUpstreamVersions(deps []*Dependency) ([]versionUpdateInfo,
                }

                if err != nil {
-                       return nil, err
+                       return nil, fmt.Errorf("dependency %s: %w", dep.Name, err)
                }

                updateAvailable, err := latestVersion.MoreSensitivelyRecentThan(currentVersion, dep.Sensitivity)
                if err != nil {
-                       return nil, err
+                       return nil, fmt.Errorf("dependency compare %s: %w", dep.Name, err)
                }

                latestVersion.Version = formatVersion(currentVersion.Version, latestVersion.Version)
FATA error during command execution: &fmt.wrapError{msg:"checking remote dependencies: dependency compare renovate: No Major.Minor.Patch elements found", err:(*fmt.wrapError)(0xc000d72800)}
  1. Don't fail on this. It's because the container check accepts version numbers "tolerantly" while the compare during upgrade does a strict parse. I think we should either always use strict, or always tolerant, or have a feature toggle if really needed.
diff --git a/dependency/version.go b/dependency/version.go
index b4d70d4..02908c2 100644
--- a/dependency/version.go
+++ b/dependency/version.go
@@ -94,13 +94,13 @@ func (a Version) MoreSensitivelyRecentThan(b Version, sensitivity VersionSensiti

        switch a.Scheme {
        case Semver:
-               aSemver, err := semver.Parse(strings.TrimPrefix(a.Version, "v"))
+               aSemver, err := semver.ParseTolerant(strings.TrimPrefix(a.Version, "v"))
                if err != nil {
                        log.Debugf("Failed to semver-parse %s", a.Version)
                        return false, err
                }

-               bSemver, err := semver.Parse(strings.TrimPrefix(b.Version, "v"))
+               bSemver, err := semver.ParseTolerant(strings.TrimPrefix(b.Version, "v"))
                if err != nil {
                        log.Debugf("Failed to semver-parse %s", b.Version)
                        return false, err

Result:

Update available for dependency renovate: 37.214 (current: 37.198.0)

How to reproduce it (as minimally and precisely as possible):

  - name: renovate
    version: 37.198.0
    scheme: semver
    upstream:
      flavour: container
      registry: ghcr.io/renovatebot/renovate
    refPaths:
      - path: components/renovate/cronjob.yaml
        match: 'renovatebot/renovate:'

Anything else we need to know?:

This didn't happen last week, so I guess it randomly occurs depending on the order of tags from the GHCR API (for example, is 37.198 returned before or after 37.198.0)

Environment:

N/A

saschagrunert commented 6 months ago

@chlunde thank you for the report. I assume we could make tolerant parsing an option to be set.

chlunde commented 5 months ago

@saschagrunert the main issue is that two different functions are used, and the strict one is after the filter. I could of course add an option but I'm not sure if that's required here.

Would it be OK to set tolerant as the default (to match what is allowed through the filter now), and then add a flag if there's a use case to run only with the strict one?

Pluies commented 5 months ago

Hmm, tough call, I agree we should have consistent handling (either all strict or all tolerant).

Potential way forward:

wdyt?

saschagrunert commented 5 months ago

Hmm, tough call, I agree we should have consistent handling (either all strict or all tolerant).

Potential way forward:

  • Set the container match to default to strict (potentially a breaking change, but as seen above the functionality is already broken)
  • Add an option on each dependency for whether the semver should be tolerant, defaulting to false

wdyt?

I'd prefer that solution. :+1:

chlunde commented 5 months ago
  • Add an option on each dependency for whether the semver should be tolerant, defaulting to false

Hmmm, there's already scheme: semver. Would scheme: semver-tolerant (I don't like the name but I don't have anything better) instead of a new flag, make sense?

Actually, there are some options per flavour already, like:

// GitLab upstream representation
type GitLab struct {                                                
    Base `mapstructure:",squash"`
... 

    // Optional: semver constraints, e.g. < 2.0.0            
    // Will have no effect if the dependency does not follow Semver
    Constraints string

func latestGitLabRelease(upstream *GitLab) (string, error) {
 ...
    semverConstraints := upstream.Constraints
    if semverConstraints == "" {
        // If no range is passed, just use the broadest possible range
        semverConstraints = DefaultSemVerConstraints
    }
Pluies commented 5 months ago

Actually, there are some options per flavour already

Yeah, that's what I had in mind 👍 adding it as an extra option there.

k8s-triage-robot commented 2 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

k8s-triage-robot commented 1 month 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

k8s-triage-robot commented 4 weeks ago

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

This bot triages issues according to the following rules:

You can:

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

/close not-planned

k8s-ci-robot commented 4 weeks ago

@k8s-triage-robot: Closing this issue, marking it as "Not Planned".

In response to [this](https://github.com/kubernetes-sigs/zeitgeist/issues/859#issuecomment-2263887007): >The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. > >This bot triages issues according to the following rules: >- After 90d of inactivity, `lifecycle/stale` is applied >- After 30d of inactivity since `lifecycle/stale` was applied, `lifecycle/rotten` is applied >- After 30d of inactivity since `lifecycle/rotten` was applied, the issue is closed > >You can: >- Reopen this issue with `/reopen` >- Mark this issue as fresh with `/remove-lifecycle rotten` >- Offer to help out with [Issue Triage][1] > >Please send feedback to sig-contributor-experience at [kubernetes/community](https://github.com/kubernetes/community). > >/close not-planned > >[1]: https://www.kubernetes.dev/docs/guide/issue-triage/ 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-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
Pluies commented 3 weeks ago

/reopen

k8s-ci-robot commented 3 weeks ago

@Pluies: Reopened this issue.

In response to [this](https://github.com/kubernetes-sigs/zeitgeist/issues/859#issuecomment-2265716789): >/reopen 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-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
Pluies commented 3 weeks ago

Will try and have a look at this soon!

Pluies commented 1 week ago

After starting implementation for the solution drafted above with a switch etc, I've changed my mind - I now think it makes more sense to use parseTolerant everywhere.

We already effectively use parseTolerant in the offending code in dependency/version.go, as we remove the leading v. And adding the whole strict / tolerant distinction in each upstream code introduces extra complexity and cognitive load on users.

Let's fix the current bug, and use parseTolerant going forwards 👍