-
Notifications
You must be signed in to change notification settings - Fork 813
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
Yaml linter #3358
Yaml linter #3358
Conversation
Build Failed 😱 Build Id: 8f325a3d-3954-4249-a3d4-9d0be609aba7 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Failed 😱 Build Id: 91ed288a-05ac-465d-a032-21a13260793e To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Failed 😱 Build Id: 25f0d0fb-42d7-49c9-8fdc-264eb71aca8d To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Succeeded 👏 Build Id: d6787463-0e7d-483e-8420-56e4df2e8483 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Build Succeeded 👏 Build Id: 3e2fd4cb-ac55-43d6-a747-e605a9683352 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
@@ -20,31 +20,31 @@ agones: | |||
memory: 4000Mi | |||
affinity: | |||
nodeAffinity: | |||
preferredDuringSchedulingIgnoredDuringExecution: null | |||
preferredDuringSchedulingIgnoredDuringExecution: |
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.
@zmerlynn,
yamlfix
formatter does these changes. Is it necessary to explicitly set preferredDuringSchedulingIgnoredDuringExecution
to null
, or is it acceptable to just omit it?
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.
I believe we explicitly need to null
it here. cc @gongmax
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.
Included null
.yamllint
Outdated
truthy: disable | ||
line-length: | ||
max: 1500 | ||
trailing-spaces: disable |
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 to ignore the trailing-spaces
error
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.
Trailing spaces seems like something we should flag?
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.
The yamllint configuration will cause it to ignore spaces at the end of lines. Can we have it?
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.
Right, I'm saying: I think we should lint that there are no trailing spaces (unless it is prohibitive for some reason).
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.
excluded it from this config file
Build Succeeded 👏 Build Id: b8602171-64da-4abe-80d4-21aa84a11982 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
@@ -4,4 +4,3 @@ rules: | |||
truthy: disable |
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.
Do we have a lot of violations of truthy
?
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 of our GitHub Actions workflow scripts use the on
keyword, and we cannot replace it with true
. Therefore, I've disabled that specific check.
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.
Because the GitHub Actions parser is looking for on
and not true
? Huh.
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.
That's right..
Build Succeeded 👏 Build Id: 977cfce9-446c-4e82-8c0f-4274de489771 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Build Succeeded 👏 Build Id: 37f546e3-cd5b-4b4b-b37a-2258fb03b526 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
build/Makefile
Outdated
@@ -489,7 +489,7 @@ build-extensions-binary-linux-arm64: $(ensure-build-image) | |||
lint: LINT_TIMEOUT ?= 15m | |||
lint: $(ensure-build-image) | |||
docker run -t -e "TERM=xterm-256color" -e "$(gomod_on)" --rm $(common_mounts) -w $(workdir_path) $(DOCKER_RUN_ARGS) $(build_tag) bash -c \ | |||
"golangci-lint run ./examples/... && golangci-lint run --timeout $(LINT_TIMEOUT) ./..." | |||
"golangci-lint run ./examples/... && golangci-lint run --timeout $(LINT_TIMEOUT) ./... && find ../ -name '*.yml' -o -name '*.yaml' | grep -v './install/' | xargs yamllint" |
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.
two things here:
- Are you sure about
..
? I think you're at the top of the repo here - I think we should prefer exclusions using the yamllint config itself rather than
find
. You can just useyamllint .
and then ignore paths in a.gitignore
style way. That allows people to just runyamllint
outside docker or the Makefile and keep the config the same. I think it would just look roughly like:
ignore:
- /install/
I wouldn't bother configuring the yaml suffixes, it looks like the default is appropriate.
Build Succeeded 👏 Build Id: 201fab39-a059-439b-bbb6-a3257dbfc09d The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: gongmax, igooch, Kalaiselvi84, zmerlynn The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Build Failed 😱 Build Id: a9b92c2f-0a48-479f-9c7e-b150ec0a59bc To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Failed 😱 Build Id: 42f9e58d-87c7-4efd-abe0-952379ddf685 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Succeeded 👏 Build Id: 0566f347-4469-4c49-af87-9d0bd2ca6eaf The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
What type of PR is this?
/kind cleanup
What this PR does / Why we need it:
Which issue(s) this PR fixes:
Closes #2443
Special notes for your reviewer: