You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@submarine.apache.org by GitBox <gi...@apache.org> on 2021/12/03 13:34:03 UTC
[GitHub] [submarine] joshvictor1024 opened a new pull request #816: SUBMARINE-1110. Add yaml linter or formatter
joshvictor1024 opened a new pull request #816:
URL: https://github.com/apache/submarine/pull/816
### What is this PR for?
<!-- A few sentences describing the overall goals of the pull request's commits.
First time? Check out the contributing guide - https://submarine.apache.org/contribution/contributions.html
-->
Use [`yamllint`](https://github.com/adrienverge/yamllint) to enforce coding style of YAML files.
Note that since `yamllint` doesn't support helm syntax, test is NOT run on files under `helm-charts` directly.
Instead, those files are rendered with `helm template` before being tested.
See script `dev-support/style-check/lint-yaml.sh` for more detail.
### What type of PR is it?
Improvement
### Todos
### What is the Jira issue?
<!-- * Open an issue on Jira https://issues.apache.org/jira/browse/SUBMARINE/
* Put link here, and add [SUBMARINE-*Jira number*] in PR title, eg. `SUBMARINE-23. PR title`
-->
https://issues.apache.org/jira/projects/SUBMARINE/issues/SUBMARINE-1110
### How should this be tested?
<!--
* First time? Setup Travis CI as described on https://submarine.apache.org/contribution/contributions.html#continuous-integration
* Strongly recommended: add automated unit tests for any new or changed behavior
* Outline any manual steps to test the PR here.
-->
A new test script is added: `dev-support/style-check/lint-yaml.sh`
### Screenshots (if appropriate)
### Questions:
* Do the license files need updating? No
* Are there breaking changes for older versions? No
* Does this need new documentation? No
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@submarine.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [submarine] joshvictor1024 commented on pull request #816: SUBMARINE-1110. Add yaml linter or formatter
Posted by GitBox <gi...@apache.org>.
joshvictor1024 commented on pull request #816:
URL: https://github.com/apache/submarine/pull/816#issuecomment-985737764
Actually, I can't quite find a YAML formatter that supports helm syntax. I'm tempted to leave autoformat for a future issue and limit this one to linting.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@submarine.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [submarine] joshvictor1024 commented on pull request #816: SUBMARINE-1110. Add yaml linter or formatter
Posted by GitBox <gi...@apache.org>.
joshvictor1024 commented on pull request #816:
URL: https://github.com/apache/submarine/pull/816#issuecomment-985657249
@KUAN-HSUN-LI thanks for the review.
1. Yes. The outputs are now properly formatted and can be seen at Actions > Check Style > View raw logs. Do ctrl+F for `.github` to see warnings regarding those files.
2. I think I just fixed the script `lint-yaml.sh` with commit `2fae698`.
Right now `submarine-cloud-v2/test/e2e/` is failing and I'm looking into it; any help is appreciated.
Also, should I mark this PR as a draft before autoformat and the corresponding docs are added?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@submarine.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [submarine] joshvictor1024 edited a comment on pull request #816: SUBMARINE-1110. Add yaml linter or formatter
Posted by GitBox <gi...@apache.org>.
joshvictor1024 edited a comment on pull request #816:
URL: https://github.com/apache/submarine/pull/816#issuecomment-985657249
@KUAN-HSUN-LI thanks for the review.
1. Yes. The outputs are now properly formatted and can be seen at Actions > Check Style > View raw logs. Do ctrl+F for `.github` to see warnings regarding those files.
2. I think I just fixed the script `lint-yaml.sh` with commit `2fae698`.
Right now `submarine-cloud-v2/test/e2e/` is failing, but it passes in my previous commit. Should I be concerned?
Also, should I mark this PR as a draft before autoformat and the corresponding docs are added? `yamllint` doesn't support formatting, and right now I'm looking at `yq`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@submarine.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [submarine] joshvictor1024 commented on pull request #816: SUBMARINE-1110. Add yaml linter
Posted by GitBox <gi...@apache.org>.
joshvictor1024 commented on pull request #816:
URL: https://github.com/apache/submarine/pull/816#issuecomment-989938467
Upon discussion, we concluded that helm syntax support and auto-formatting are still desired. Can we put this PR on hold for now?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@submarine.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [submarine] joshvictor1024 commented on pull request #816: SUBMARINE-1110. Add yaml linter or formatter
Posted by GitBox <gi...@apache.org>.
joshvictor1024 commented on pull request #816:
URL: https://github.com/apache/submarine/pull/816#issuecomment-987678090
It seems that `yq` doesn't and won't support helm syntax either. In [its issue #636](https://github.com/mikefarah/yq/issues/636) it's mentioned that support is impossible.
I'll limit this PR to just a YAML linter. I'll make changes to `submarine/submarine-cloud-v2/docs/developer-guide.md` accordingly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@submarine.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [submarine] joshvictor1024 commented on pull request #816: SUBMARINE-1110. Add yaml linter or formatter
Posted by GitBox <gi...@apache.org>.
joshvictor1024 commented on pull request #816:
URL: https://github.com/apache/submarine/pull/816#issuecomment-985750179
Another issue I found is that [Prettier](https://prettier.io/), which is used in `submarine-cloud-v2/`, will always break linting for multiline flow sequences (the `[]` list). An example would be line 64 of [`submarine-cloud-v2/artifacts/submarine/submarine-mlflow.yaml`](https://github.com/apache/submarine/blob/1133f2bb47b7eda81fbd181e51d474dc323f16df/submarine-cloud-v2/artifacts/submarine/submarine-mlflow.yaml#L64)
```yaml
# output of prettier (4 spaces for items inside "[]", 2 spaces anywhere else)
Item:
Item-1: [
"item-a",
"item-b",
]
Item-2: item-a
Item-3: item-a
# expected: consistent indentation (2 spaces)
Item:
Item-1: [
"item-a",
"item-b",
]
Item-2: item-a
Item-3: item-a
# work-around: no flow styles, only block styles?
Item:
Item-1:
- "item-a",
- "item-b",
Item-2: item-a
Item-3: item-a
```
We might need to give up Prettier if we want to use linting for indentation...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@submarine.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [submarine] joshvictor1024 commented on pull request #816: SUBMARINE-1110. Add yaml linter or formatter
Posted by GitBox <gi...@apache.org>.
joshvictor1024 commented on pull request #816:
URL: https://github.com/apache/submarine/pull/816#issuecomment-986051817
The one thing that prettier (and yamllint) fails to parse is the `{{-` `-}}` syntax, so it won't work with `helm-charts/submarine/charts/traefik/templates/deployment.yaml` for example.
Other than that, we could switch yamllint for prettier.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@submarine.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [submarine] KUAN-HSUN-LI edited a comment on pull request #816: SUBMARINE-1110. Add yaml linter or formatter
Posted by GitBox <gi...@apache.org>.
KUAN-HSUN-LI edited a comment on pull request #816:
URL: https://github.com/apache/submarine/pull/816#issuecomment-987591792
> The one thing that prettier (and yamllint) fails to parse is the `{{-` `-}}` syntax, so it won't work with `helm-charts/submarine/charts/traefik/templates/deployment.yaml` for example.
>
> Other than that, we could switch yamllint for prettier.
Got it.
@joshvictor1024 Would you handle the `yq` or other formatter that can format helm syntax here?
Or is this PR just for a YAML linter?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@submarine.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [submarine] KUAN-HSUN-LI commented on pull request #816: SUBMARINE-1110. Add yaml linter or formatter
Posted by GitBox <gi...@apache.org>.
KUAN-HSUN-LI commented on pull request #816:
URL: https://github.com/apache/submarine/pull/816#issuecomment-986045928
Can prettier with the command `--no-bracket-spacing` do well in helm template?
If it could, we can change the link by using `prettier --check --no-bracket-spacing` and write a autoformat script using prettier
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@submarine.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [submarine] KUAN-HSUN-LI commented on pull request #816: SUBMARINE-1110. Add yaml linter or formatter
Posted by GitBox <gi...@apache.org>.
KUAN-HSUN-LI commented on pull request #816:
URL: https://github.com/apache/submarine/pull/816#issuecomment-987591792
> The one thing that prettier (and yamllint) fails to parse is the `{{-` `-}}` syntax, so it won't work with `helm-charts/submarine/charts/traefik/templates/deployment.yaml` for example.
>
> Other than that, we could switch yamllint for prettier.
Got it.
@joshvictor1024 Would you handle the `yq` or other formatter that can format helm syntax?
Or is this PR just for a YAML linter?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@submarine.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org