Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🌱 verify go modules are in sync with upstream k/k #2774

Conversation

alexandremahdhaoui
Copy link
Contributor

@alexandremahdhaoui alexandremahdhaoui commented Apr 14, 2024

This PR should address the following issue #2769.

Changes

  • add gomodcheck:
    • Parse and compares k/k direct dependencies to controller-runtime's direct deps.
    • If any version diffs is found, returns a payload describing the diffs and exit 1.
    • The user may exclude packages by passing them as arguments.
  • add gomodcheck to the verify-modules make target.

Fixes #2769

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 14, 2024
@k8s-ci-robot
Copy link
Contributor

Hi @alexandremahdhaoui. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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/test-infra repository.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 14, 2024
@alexandremahdhaoui alexandremahdhaoui force-pushed the sync_go_mod_kubernetes_kubernetes branch 2 times, most recently from 714596f to 86ad2dc Compare April 14, 2024 09:47
@alexandremahdhaoui
Copy link
Contributor Author

Here is a list of currently out of sync dependencies:

{
  "outOfSyncModules": [
    {
      "name": "github.com/prometheus/client_golang",
      "version": "v1.17.0",
      "upstreamVersion": "v1.16.0"
    },
    {
      "name": "github.com/prometheus/client_model",
      "version": "v0.4.1-0.20230718164431-9a2bf3000d16",
      "upstreamVersion": "v0.4.0"
    },
    {
      "name": "golang.org/x/sys",
      "version": "v0.19.0",
      "upstreamVersion": "v0.18.0"
    },
    {
      "name": "go.uber.org/zap",
      "version": "v1.27.0",
      "upstreamVersion": "v1.26.0"
    },
    {
      "name": "sigs.k8s.io/yaml",
      "version": "v1.4.0",
      "upstreamVersion": "v1.3.0"
    }
  ]
}

@sbueringer
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 15, 2024
@sbueringer
Copy link
Member

sbueringer commented Apr 15, 2024

Looks like we can downgrade go.uber.org/zap, golang.org/x/sys, sigs.k8s.io/yaml

I assume this is also what we would want to do to play it absolutely safe (consumers of CR can still bump to higher versions if they are sure).

Not sure what is going on with the prometheus/client_* libs. go mod tidy automatically bumps them. I think that is also how I somehow downgraded to the wrong version after bringing them in sync with k/k on my last PR. Have to take a closer look.

EDIT: Turns out I just also had to downgrade github.com/prometheus/common v0.45.0 (which was an indirect dependency bumped by one of the bump PRs)

(cc @vincepri @alvaroaleman)

@sbueringer
Copy link
Member

PR to fix the current open findings: #2776

@alexandremahdhaoui alexandremahdhaoui force-pushed the sync_go_mod_kubernetes_kubernetes branch 2 times, most recently from c2deb09 to 75371f3 Compare April 16, 2024 21:01
@alexandremahdhaoui
Copy link
Contributor Author

alexandremahdhaoui commented Apr 16, 2024

The modules listed below are now flagged as out of sync:

{
  "outOfSyncModules": [
    {
      "name": "github.com/go-logr/logr",
      "version": "v1.4.1",
      "upstreams": [
        {
          "ref": "k8s.io/utils",
          "version": "v1.2.0"
        }
      ]
    },
    {
      "name": "k8s.io/klog/v2",
      "version": "v2.120.1",
      "upstreams": [
        {
          "ref": "k8s.io/utils",
          "version": "v2.80.1"
        }
      ]
    }
  ]
}

@sbueringer
Copy link
Member

sbueringer commented Apr 17, 2024

Did I understand this correctly that we are now finding inconsistencies between our dependencies?

E.g.

  • k8s.io/klog/v2
    • we use: v2.120.1
    • k/k uses: v2.120.1 (I'm aware there is no "k/k" go module :))
    • k8s.io/utils v0.0.0-20230726121419-3b25d923346b uses v2.80.1

Given that we are using the same k8s.io/utils as k/k go.mod I don't think it's fixable for us to ensure all of these dependencies sync up.

I think we have to restrict the dependencies we go through (e.g. focusing on that some "core/main" k/k modules are in sync with us, like k8s.io/client-go)

We shouldn't become responsible to ensure that all k/k dependencies are using the same transitive dependencies

@sbueringer
Copy link
Member

@vincepri I think this is a consequence of now looking at the go mod graph instead of looking at the k/k go.mod file. WDYT? (I would suggest to just compare dependencies between CR and a few "main/core" go modules of k/k)

@alexandremahdhaoui
Copy link
Contributor Author

alexandremahdhaoui commented Apr 17, 2024

Edit: NVM, we can just remove k8s.io/utils from the upstreamRefs in hack/.gomodcheck.yaml

@alexandremahdhaoui
Copy link
Contributor Author

NB: I was wondering if it can be considered okay if I create a personal repo to write an enhanced version of gomodcheck and release it there? The tool can be useful and re-used for other projects.

@sbueringer
Copy link
Member

NB: I was wondering if it can be considered okay if I create a personal repo to write an enhanced version of gomodcheck and release it there? The tool can be useful and re-used for other projects.

Definitely okay to do that, but we're very hesitant to add dependencies to personal repos. So we would continue to use our local version I think

@sbueringer
Copy link
Member

@alexandremahdhaoui I tried to use it on top of #2786 and I'm getting some strange results:

 make verify-modules
go mod tidy
cd hack/tools; go mod tidy
cd /Users/buringerst/code/src/sigs.k8s.io/controller-runtime/tools/setup-envtest; go mod tidy
cd /Users/buringerst/code/src/sigs.k8s.io/controller-runtime/examples/scratch-env; go mod tidy
/Users/buringerst/code/src/sigs.k8s.io/controller-runtime/hack/tools/bin/gomodcheck /Users/buringerst/code/src/sigs.k8s.io/controller-runtime/hack/.gomodcheck.yaml
{
  "outOfSyncModules": [
    {
      "name": "k8s.io/api",
      "version": "v0.30.0",
      "upstreamVersion": "v0.0.0"
    },
    {
      "name": "k8s.io/client-go",
      "version": "v0.30.0",
      "upstreamVersion": "v0.0.0"
    },
    {
      "name": "github.com/onsi/gomega",
      "version": "v1.32.0",
      "upstreamVersion": "v1.31.0"
    },
    {
      "name": "k8s.io/component-base",
      "version": "v0.30.0",
      "upstreamVersion": "v0.0.0"
    },
    {
      "name": "github.com/onsi/ginkgo/v2",
      "version": "v2.17.1",
      "upstreamVersion": "v2.15.0"
    },
    {
      "name": "k8s.io/apiextensions-apiserver",
      "version": "v0.30.0",
      "upstreamVersion": "v0.0.0"
    },
    {
      "name": "k8s.io/apimachinery",
      "version": "v0.30.0",
      "upstreamVersion": "v0.0.0"
    },
    {
      "name": "k8s.io/apiserver",
      "version": "v0.30.0",
      "upstreamVersion": "v0.0.0"
    }
  ]
}
make: *** [verify-modules] Error 1

@alexandremahdhaoui
Copy link
Contributor Author

alexandremahdhaoui commented Apr 18, 2024

@sbueringer it looks like you cherry-picked an old commit of this branch.
NB: I've rebased onto main and tests pass.

Copy link
Member

@sbueringer sbueringer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good. Just a few smaller findings

hack/.gomodcheck.yaml Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
hack/tools/cmd/gomodcheck/main.go Show resolved Hide resolved
hack/tools/cmd/gomodcheck/main.go Outdated Show resolved Hide resolved
hack/tools/cmd/gomodcheck/main.go Outdated Show resolved Hide resolved
alexandremahdhaoui and others added 3 commits April 27, 2024 11:05
This commit addresses issues were go modules aren't in sync with
upstream k/k by adding these changes:
- add `tools/cmd/gomodcheck/main.go` to:
  - Parse and compares k/k dependencies to controller-runtime's ones.
  - If any version diffs is found, returns a payload describing the diffs
    and exit 1.
  - The user may exclude packages by passing them as arguments.
- extend the `verify-modules` make target with `gomodcheck`.
Co-authored-by: Stefan Büringer <4662360+sbueringer@users.noreply.github.com>
Signed-off-by: Alexandre Mahdhaoui <alexandre.mahdhaoui@gmail.com>
@alexandremahdhaoui alexandremahdhaoui force-pushed the sync_go_mod_kubernetes_kubernetes branch 3 times, most recently from 0ad4c4c to 105349a Compare April 27, 2024 10:08
Signed-off-by: Alexandre Mahdhaoui <alexandre.mahdhaoui@gmail.com>
@sbueringer
Copy link
Member

Very nice. Thank you!

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 29, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 3881f0ce785eddfc99d0a4fd7e16a93444ea8bd9

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexandremahdhaoui, sbueringer

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 29, 2024
@k8s-ci-robot k8s-ci-robot merged commit f6b23b1 into kubernetes-sigs:main Apr 29, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a validation script to verify go.mod is in sync with k/k
4 participants