-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
fix breaking examples #5024
fix breaking examples #5024
Conversation
This PR has multiple commits, and the default merge method is: merge. 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. |
@@ -4,12 +4,11 @@ resources: | |||
- https://raw.githubusercontent.com/kubernetes-sigs/kustomize/master/examples/wordpress/wordpress/deployment.yaml | |||
- https://github.com/knative/serving/releases/download/v0.12.0/serving.yaml # redirects to s3 | |||
patches: |
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.
removed legacy patch syntax is used.
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.
This is used in other examples - is it worth updating those as well?
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.
@cailynse
I think I searched and fixed other examples where the removed legacy patch was used.
Could you find other places for it was used?
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.
Sorry! I misunderstood, I was thinking patchesStrategicMerge
which is being used in many places
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.
Don't worry!
@@ -0,0 +1,8 @@ | |||
resources: |
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.
error caused by kustomization.yaml
file is missing.
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.
Does this mean the example in the issue never worked?
Just saw Katrina's comment!
Are the fixes taken from the tutorial snippets? Should we update them there instead? I don't feel strongly about it, but the tutorial is telling the user to create these files, isn't it? https://github.com/kubernetes-sigs/kustomize/tree/master/examples/springboot |
That is right. kustomize/examples/springboot/README.md Lines 107 to 119 in 0fd385d
|
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.
Please fix the examples, 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
@KnVerey @cailynse The README.md tutorial works independently of kustomize build
on the roots under the springboot
folder. The tutorial works because it has the user create the kustomization.yaml
file that's missing under base
. The tutorial also has the user create the .properties and patch files from scratch, though the content of these files is based on that of the files under overlays/production
.
We could, optionally, have the tutorial curl
the files from the springboot
folders instead of providing the snippets that @koba1t mentions if we find them repetitive. I believe only patch.yaml
file has diverged from the tutorial. The tutorial instructions, however, do not contain the legacy fields because they're created using the kustomize edit
commands.
@@ -0,0 +1,5 @@ | |||
app.name=Staging Kinflate Demo |
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.
nit: The README.md specifies an application.properties
that only has 1 line:
app.name=Kustomize Demo
Apparently the justification there is that maintenance is easier if we separate the credentials out into a separate file: https://github.com/kubernetes-sigs/kustomize/blob/master/examples/springboot/README.md?plain=1#L95-L99.
Feel free to ignore this comment. I don't think it's important, only an observation.
/label tide/merge-method-squash |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: annasong20, koba1t, natasha41575, tonglil 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 |
* fix examples?springboot with no kustomization.yaml * fix examples/loadHttp with legacy patch syntax
fix breaking examples in
./examples
dir.fix #4997