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

doc: update example to use tag v3.3.1 #5265

Merged
merged 1 commit into from
Oct 6, 2023
Merged

doc: update example to use tag v3.3.1 #5265

merged 1 commit into from
Oct 6, 2023

Conversation

rodrigc
Copy link
Contributor

@rodrigc rodrigc commented Aug 11, 2023

The current example uses tag v1.0.6 .

However, if you use a kustomization file that looks like this:

---
apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization

resources:
  - https://github.com/kubernetes-sigs/kustomize//examples/multibases/dev/?ref=v1.0.6

kustomize build will work, but give a warning:

# Warning: 'bases' is deprecated. Please use 'resources' instead. Run 'kustomize edit fix' to update your Kustomization automatically.

For people like me who are newbies to customize, this was confusing.
It turns out that the underlying code in the kustomize repo has been fixed to use the latest syntax that does not emit deprecation warnings. So just use the newer tag.

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

Welcome @rodrigc!

It looks like this is your first PR to kubernetes-sigs/kustomize 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kustomize has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot
Copy link
Contributor

Hi @rodrigc. 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/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 11, 2023
Copy link

@Aisuko Aisuko left a comment

Choose a reason for hiding this comment

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

approved

@Aisuko
Copy link

Aisuko commented Aug 22, 2023

/ok-to-test
/test all

@k8s-ci-robot
Copy link
Contributor

@Aisuko: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/test

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.

@Aisuko
Copy link

Aisuko commented Aug 22, 2023

/ok-to-test

@k8s-ci-robot
Copy link
Contributor

@Aisuko: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/ok-to-test

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.

@Aisuko
Copy link

Aisuko commented Aug 22, 2023

/assign

@Aisuko
Copy link

Aisuko commented Aug 25, 2023

/ok-to-test

@k8s-ci-robot
Copy link
Contributor

@Aisuko: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/ok-to-test

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.

@Aisuko
Copy link

Aisuko commented Aug 25, 2023

/lgtm

@k8s-ci-robot
Copy link
Contributor

@Aisuko: changing LGTM is restricted to collaborators

In response to this:

/lgtm

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.

@rodrigc
Copy link
Contributor Author

rodrigc commented Aug 25, 2023

@Aisuko Thanks!

@Aisuko
Copy link

Aisuko commented Sep 5, 2023

Hi, @koba1t may you please help us on this one? thanks.

@koba1t
Copy link
Member

koba1t commented Sep 6, 2023

Hi @rodrigc
Thanks for your contribution!

I think tag v3.3.1 is a super old release tag. We have added many changes since that tag was released.
Why you don't use the latest release tags?

@rodrigc
Copy link
Contributor Author

rodrigc commented Sep 6, 2023

@koba1t I just picked v3.3.1 because it was an older tag that the examples still worked with,
and did not produce a warning like the existing usage of v1.0.6 which is very old.

Which of these tags do you recommend I use:
https://github.com/kubernetes-sigs/kustomize/tags

It looks like after v3.3.1 the format of the tags have changed

@koba1t
Copy link
Member

koba1t commented Sep 7, 2023

Which of these tags do you recommend I use:

I suggest using the latest Kustomzie release tag.

@rodrigc rodrigc changed the title doc: update example to use tag v3.3.1 doc: update example to use tag kustomize/v5.1.1 Sep 7, 2023
@rodrigc
Copy link
Contributor Author

rodrigc commented Sep 7, 2023

@koba1t OK, I made the changed the tag to kustomize/v5.1.1.

See rendered version of the file here: https://github.com/kubernetes-sigs/kustomize/blob/806f11341e2ac42fe642cfe5a8dad7f1d72b84d4/examples/remoteBuild.md

@koba1t
Copy link
Member

koba1t commented Sep 7, 2023

@rodrigc

I found an error when testing using kustomization.yaml file on this document.
Could you fix or try to find out what is the cause?

$ cat kustomization.yaml
resources:
- https://github.com/kubernetes-sigs/kustomize//examples/multibases?ref=kustomize/v5.1.1
namePrefix: remote-
$ kustomize build .
Error: accumulating resources: accumulation err='accumulating resources from 'https://github.com/kubernetes-sigs/kustomize//examples/multibases?ref=kustomize/v5.1.1': URL is a git repository': hit 27s timeout running '/usr/bin/git submodule update --init --recursive'

@rodrigc
Copy link
Contributor Author

rodrigc commented Sep 7, 2023

I tried to reproduce your problem.
I ran kustomize build a few times, and got the same error that you did.

But then I ran it again and it worked, and I got:

apiVersion: v1
kind: Pod
metadata:
  labels:
    app: myapp
  name: remote-cluster-a-dev-myapp-pod
spec:
  containers:
  - image: nginx:1.7.9
    name: nginx
---
apiVersion: v1
kind: Pod
metadata:
  labels:
    app: myapp
  name: remote-cluster-a-prod-myapp-pod
spec:
  containers:
  - image: nginx:1.7.9
    name: nginx
---
apiVersion: v1
kind: Pod
metadata:
  labels:
    app: myapp
  name: remote-cluster-a-staging-myapp-pod
spec:
  containers:
  - image: nginx:1.7.9
    name: nginx

I'm not sure what the issue is. It might be a performance issue against GitHub.

@rodrigc
Copy link
Contributor Author

rodrigc commented Sep 7, 2023

Seems related to this: #3742

Does adding a &timeout value to your URL help in your case?

Like this:

resources:
  - https://github.com/kubernetes-sigs/kustomize//examples/multibases?timeout=120&ref=kustomize/v5.1.1
  
namePrefix: remote-

@koba1t
Copy link
Member

koba1t commented Sep 7, 2023

@rodrigc

Does adding a &timeout value to your URL help in your case?

Yes, I got success for the build when I added &timeout.

$ cat kustomization.yaml
resources:
- https://github.com/kubernetes-sigs/kustomize//examples/multibases?ref=kustomize/v5.1.1&timeout=90s
namePrefix: remote-

@rodrigc
Copy link
Contributor Author

rodrigc commented Sep 8, 2023

You want me to add a timeout value to all the URL's in this patch?

Since this is just modifying a doc, might as well have a doc that the average user can try, and not run into GitHub errors?

@rodrigc
Copy link
Contributor Author

rodrigc commented Sep 8, 2023

I added a timeout, so this should work with github

@koba1t
Copy link
Member

koba1t commented Sep 14, 2023

I added a timeout, so this should work with github

Thanks!
/lgtm

I can't understand why we take more time to clone, but I think there is no need to fix it here.

/cc @natasha41575 @annasong20

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 14, 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 Oct 4, 2023
@@ -48,7 +48,7 @@ one pod in the output:

<!-- @remoteOverlayBuild @testAgainstLatestRelease -->
```
target="https://github.com/kubernetes-sigs/kustomize//examples/multibases/dev/?ref=v1.0.6"
target="https://github.com/kubernetes-sigs/kustomize//examples/multibases/dev/?timeout=120&ref=kustomize/v5.1.1"
Copy link
Contributor

@natasha41575 natasha41575 Oct 6, 2023

Choose a reason for hiding this comment

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

When I run this, the timeout is too small for me. I had to up the timeout to 3 minutes on my machine, so I'm not sure that this timeout will be universally ok for all users. I believe the reason that v5 takes so much longer to clone than v1 is because the repo is much, much bigger now.

I think it will be very confusing for users (and a bad user experience) who run this command and see it hang for over a minute while kustomize tries to clone the large repo. I myself thought it was broken at first and that there must be something wrong with the syntax or example until it timed out at 2 minutes.

I see in a previous thread that you had it originally using v3.3.1. When I run that, it returns instantly and without printing the confusing warning. I think that will be the best user experience for users who follow these docs. After 3.3.1, we changed kustomize to have multiple modules, hence why the repository suddenly increased in size so much.

Really sorry for the back and forth (I know it's annoying) but let's stick with v3.3.1 for this (fyi @koba1t). Once you make that change, I can lgtm and approve this.

Signed-off-by: Craig Rodrigues <rodrigc@crodrigues.org>
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 6, 2023
@rodrigc rodrigc requested review from natasha41575 and Aisuko October 6, 2023 18:39
@rodrigc rodrigc changed the title doc: update example to use tag kustomize/v5.1.1 doc: update example to use tag v3.3.1 Oct 6, 2023
@rodrigc
Copy link
Contributor Author

rodrigc commented Oct 6, 2023

OK, I changed the tag back to v3.3.1

@natasha41575
Copy link
Contributor

/lgtm
/approve

Thanks! Bot will merge this once the tests are passing

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Aisuko, natasha41575, rodrigc

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 Oct 6, 2023
@k8s-ci-robot k8s-ci-robot merged commit 852739c into kubernetes-sigs:master Oct 6, 2023
@rodrigc rodrigc deleted the update-remote-example branch October 6, 2023 19:26
@greenkiwi
Copy link

Is there any option to specify a different default timeout? Rather than having to put the timeout in every URL?

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/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
Development

Successfully merging this pull request may close these issues.

6 participants