-
Notifications
You must be signed in to change notification settings - Fork 8.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
Indent values.yaml using 2 instead of 4 spaces #9656
Conversation
This issue is currently awaiting triage. If Ingress contributors determines this is a relevant issue, they will accept it by applying the The 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. |
Welcome @Joibel! |
Hi @Joibel. Thanks for your PR. I'm waiting for a kubernetes 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. |
@Joibel thanks for the contribution. Can you kindly check if you have empty lines in your fork/clone or is that a github view problem. |
I believe it is github finding some lines that it can match up (even though they shouldn't in this case) which actually come from a very different part of the file. Try viewing the diff in a different viewer. |
I checked your fork and seems good. I have one more request because this is
such a critical piece. I was hoping you can copy paste a install log and
story, using your values file. Like minikube or kind cluster and do a helm
install that clearly and distinctly uses your fork's values file and then
shows a working controller. Just CLI and logs and a curl.
…On Mon, 20 Feb, 2023, 10:42 pm Alan Clucas, ***@***.***> wrote:
I believe it is github finding some lines that it can match up (even
though they shouldn't in this case) which actually come from a very
different part of the file. Try viewing the diff in a different viewer.
git diff HEAD~ -b shows there are no non-whitespace changes.
—
Reply to this email directly, view it on GitHub
<#9656 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABGZVWVPDQ7ZUZ2CAXKYYVLWYOQYZANCNFSM6AAAAAAVBY6RHE>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
/assign |
Install log:
Logs from the controller:
This was a curl to the ingress port.
Tests performed on minikube with minikube tunnel Another test was:
Showing no differences |
Thanks for your contributions. In fact, this file is currently automatically generated by the program, so directly modifying this file cannot really solve the problem. ingress-nginx/magefiles/helm.go Lines 122 to 154 in d21ae2d
|
Ok, I've changed the PR to generate values with an indentation of 2. I've left in the modified |
@Joibel Let's remove the values.yaml update, it won't get pushed since were not releasing a new chart; on the next release, with the update to the mage code, it will get updated; thanks for the contribution. |
Signed-off-by: Alan Clucas <[email protected]>
Done. |
/retest |
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.
/ok-to-test
/lgtm
/label tide/merge-method-squash |
I will re-run all CI jobs. |
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 tests passed.
/unhold
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Joibel, tao12345666333 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 |
What this PR does / why we need it:
This is much more conventional, and how it was here until commit 80fd69e which didn't appear to intentionally change this. Fixes #9655
Types of changes
Mostly just convention and standards
Which issue/s this PR fixes
fixes #9655
How Has This Been Tested?
The yaml is functionally identical.
See commentary for more testing - deployment is successful and resulting values.yaml is identical barring whitespace.
Checklist:
None required
Does my pull request need a release note?