-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
✨ Updated Makefile to support go install kustomize #3398
✨ Updated Makefile to support go install kustomize #3398
Conversation
Welcome @lauchokyip! |
Hi @lauchokyip. 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 Once the patch is verified, the new status will be reflected by the 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. |
834b149
to
0108d46
Compare
/lgtm |
/cc @camilamacedo86 Can you PTAL? thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/ok-to-test
This looks good. @lauchokyip. Thanks for working on this. This would also need an update in the migration guide in the KB docs. In this section to be specific (https://kubebuilder.io/migration/migration_guide_gov3_to_gov4.html?highlight=v3,to,v4#initialize-a-gov4-project). Would you mind taking a look and changing them.
0108d46
to
e42cc13
Compare
@varshaprasad96 Thanks! added over here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Thanks @lauchokyip
@camilamacedo86 @everettraven @Kavinjsir can we have another lgtm to merge this?
/retest |
@@ -141,15 +141,14 @@ ENVTEST ?= $(LOCALBIN)/setup-envtest | |||
KUSTOMIZE_VERSION ?= v5.0.1 | |||
CONTROLLER_TOOLS_VERSION ?= v0.12.0 | |||
|
|||
KUSTOMIZE_INSTALL_SCRIPT ?= "https://raw.githubusercontent.com/kubernetes-sigs/kustomize/master/hack/install_kustomize.sh" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please have your branch rebased with master and run make generate
to update the samples and docs?
Also, please when you push the PR to update this one can you have the commits squashed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My apology, I forgot to setup upstream
using git remote
😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all fine. Thank you
e42cc13
to
f4b2f49
Compare
/retest |
Great job 🥇 But sorry it still missing the rebase with master /lgtm cancel |
f4b2f49
to
d03ea89
Compare
~I think there is some bug exists on I already rebased with upstream master as below, and run
|
Turned out I was using old |
d03ea89
to
604813a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AlmogBaku, camilamacedo86, lauchokyip, varshaprasad96 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 |
This change will change the method of installation from downloading the script to using
go install
Fixes #2822