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

Feature/dependency pinning and update automation #5451

Conversation

antoooks
Copy link
Contributor

@antoooks antoooks commented Nov 16, 2023

  • Enable gorepomod to work outside $GOSRC, by adding --local
  • Enable auto fetch update on gorepomod list and gorepomod pin
  • Enable gorepomod to work on fork repository
  • Ensure backward compatibility and manual execution still works
  • Add test for local flag
  • Enable release for patch
  • Add backward compatibility for unpin

Note: implementation done based on assumption that the release process will produce release-{module}-vX.Y.Z release branch naming. And this branch is created manually when releasing which will become trigger to the CI steps

Addresses #5404

@k8s-ci-robot
Copy link
Contributor

This PR has multiple commits, and the default merge method is: merge.
You can request commits to be squashed using the label: tide/merge-method-squash

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 cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 16, 2023
@k8s-ci-robot
Copy link
Contributor

Hi @antoooks. 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.

@antoooks antoooks marked this pull request as draft November 16, 2023 07:04
@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 16, 2023
@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 20, 2023
@antoooks antoooks force-pushed the feature/dependency-pinning-and-update-automation branch 2 times, most recently from 09546a7 to b94888f Compare November 21, 2023 16:58
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 21, 2023
@antoooks antoooks marked this pull request as ready for review November 25, 2023 15:32
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 25, 2023
@natasha41575
Copy link
Contributor

/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 Nov 27, 2023
@koba1t
Copy link
Member

koba1t commented Nov 28, 2023

/assign koba1t

@koba1t
Copy link
Member

koba1t commented Nov 29, 2023

@antoooks
Looks like CI failed. Could you fix the Lint error?

Copy link
Member

@koba1t koba1t left a comment

Choose a reason for hiding this comment

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

Thanks for your work!
I reviewed this PR. Please check my comments.

cmd/gorepomod/internal/git/runner.go Outdated Show resolved Hide resolved
cmd/gorepomod/internal/repo/dotgitdata.go Outdated Show resolved Hide resolved
cmd/gorepomod/internal/utils/utils.go Outdated Show resolved Hide resolved
@koba1t
Copy link
Member

koba1t commented Nov 29, 2023

This code looks almost good!
I will check the behavior of the gorepomod command after a few small comments are fixed.

@antoooks
Copy link
Contributor Author

antoooks commented Dec 7, 2023

It's weird, I ran lint locally and it came out fine. But if it ran on CI it throws many check errors

Screenshot 2023-12-07 at 8 12 31 AM

@koba1t do you know how I can reproduce same linting rule as CI?

@koba1t
Copy link
Member

koba1t commented Dec 7, 2023

hmm...
I think GitHub Action is run CI after 'merge master'.
Could you try 'rebase master'?

* add managerfactory handling for local flag
* add shortName handling for local flag
* add dot git file handling for local flag
* add tests
@antoooks antoooks force-pushed the feature/dependency-pinning-and-update-automation branch from 5335512 to 276cc0c Compare December 7, 2023 08:22
@koba1t
Copy link
Member

koba1t commented Dec 16, 2023

/label tide/merge-method-squash

@k8s-ci-robot k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Dec 16, 2023
@koba1t
Copy link
Member

koba1t commented Dec 16, 2023

Hi @antoooks
I try to pin and unpin kyaml without go/src dir. So looks like the go.mod file was broken.
Could you investigate this?

kob@Yugos-MacBook-Air kustomize % git diff HEAD
diff --git a/api/go.mod b/api/go.mod
index c350266f7..66d98517b 100644
--- a/api/go.mod
+++ b/api/go.mod
@@ -36,4 +36,4 @@ require (
        gopkg.in/yaml.v3 v3.0.1 // indirect
 )

-replace sigs.k8s.io/kustomize/kyaml => ../kyaml
+replace kustomize/kyaml => ../kyaml
diff --git a/api/go.sum b/api/go.sum
index efb9f252a..30ee0ec8c 100644

@antoooks
Copy link
Contributor Author

Hi @antoooks I try to pin and unpin kyaml without go/src dir. So looks like the go.mod file was broken. Could you investigate this?

kob@Yugos-MacBook-Air kustomize % git diff HEAD
diff --git a/api/go.mod b/api/go.mod
index c350266f7..66d98517b 100644
--- a/api/go.mod
+++ b/api/go.mod
@@ -36,4 +36,4 @@ require (
        gopkg.in/yaml.v3 v3.0.1 // indirect
 )

-replace sigs.k8s.io/kustomize/kyaml => ../kyaml
+replace kustomize/kyaml => ../kyaml
diff --git a/api/go.sum b/api/go.sum
index efb9f252a..30ee0ec8c 100644

@koba1t Nice catch, I assumed we use separate release branch so I didn't work on unpin, but I think I should to support backward compatibility. Will fix it now, thanks Yugo-san

@stormqueen1990
Copy link
Member

/uncc

@k8s-ci-robot k8s-ci-robot removed the request for review from stormqueen1990 December 18, 2023 01:49
@koba1t
Copy link
Member

koba1t commented Jan 16, 2024

Nice catch, I assumed we use separate release branch so I didn't work on unpin, but I think I should to support backward compatibility.

Thanks, @antoooks!
Maybe this command is working on my computer. It looks good to me!
Let's try it in the next release.

/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 Jan 16, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: antoooks, koba1t

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 Jan 16, 2024
@k8s-ci-robot k8s-ci-robot merged commit ab519fd into kubernetes-sigs:master Jan 16, 2024
9 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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Development

Successfully merging this pull request may close these issues.

5 participants