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

ci: add fix-yamllint rule in Makefile #15966

Merged
merged 1 commit into from
Jun 5, 2023
Merged

Conversation

tao12345666333
Copy link
Contributor

@tao12345666333 tao12345666333 commented May 26, 2023

Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.

follow-up #15955

Two rules have been specified for yamlfmt here:

  • Automatically add the header --- to YAML files.
  • Preserve line breaks.

ref: https://github.com/google/yamlfmt/blob/main/docs/config-file.md#configuration-1

Makefile Outdated Show resolved Hide resolved
.yamlfmt Outdated
@@ -0,0 +1,4 @@
formatter:
Copy link
Member

Choose a reason for hiding this comment

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

Should we move this configuration file out of the root directory to reduce initial clutter?

It's possible to specify the file location via https://github.com/google/yamlfmt/blob/main/docs/command-usage.md#configuration-flags

It could sit under /tools or /tests instead perhaps. Just an idea given we have been discussing tidying root directory.

In hindsight we could actually do the same for yamllint https://yamllint.readthedocs.io/en/stable/configuration.html.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your suggestion. Using parameters can indeed reduce the content of the root directory.

There are several reasons why I use configuration files:

  • These configuration files are hidden by default, and most people will not see them directly;
  • The names of these configuration files are very intuitive, and their functions can be understood through their names.

The difference from #15778 is that we expect yamllint and yamlfmt to be triggered through make, so there is no need to touch these configuration files.
But the Procfile file is included in the documentation, and people need to understand its function and significance.

If modifications are needed, I can make adjustments. yamlfmt passes parameters through flags, and the configuration file for yamllint is located in either the tools or scripts directory.

Copy link
Member

@jmhbnz jmhbnz May 30, 2023

Choose a reason for hiding this comment

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

Thanks @tao12345666333, my preference would be to:

  1. Move .yamllint and .yamlfmt to tools/...
  2. Update Makefile as follows:
verify-yamllint:
-	yamllint .
+      yamllint --config-file tools/.yamllint .
fix-yamllint:
ifeq (, $(shell which yamlfmt))
	$(shell go install github.com/google/yamlfmt/cmd/yamlfmt@latest)
endif
-	yamlfmt .
+      yamlfmt -conf tools/.yamlfmt .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed!

Copy link
Member

Choose a reason for hiding this comment

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

Agree with the proposed direction. @jmhbnz Can you file an issue to consider moving other dotfiles?

Copy link
Member

Choose a reason for hiding this comment

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

FYI there is also hack directory borrowed from kubernetes that serves similar purpose.

Copy link
Member

Choose a reason for hiding this comment

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

Issue raised #15987. @tao12345666333 you are most welcome to work on that one next once this merges, just let me know if you are interested and I can assign it to you :)

Copy link
Member

Choose a reason for hiding this comment

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

Don't like the idea of putting the config files under hack.

#15987 (comment)

Makefile Outdated Show resolved Hide resolved
Copy link
Member

@jmhbnz jmhbnz left a comment

Choose a reason for hiding this comment

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

LGTM

Makefile Outdated Show resolved Hide resolved
Two rules have been specified for yamlfmt here:
* Automatically add the header `---` to YAML files.
* Preserve line breaks.

ref:
https://github.com/google/yamlfmt/blob/main/docs/config-file.md#configuration-1

Signed-off-by: Jintao Zhang <zhangjintao9020@gmail.com>
Co-authored-by: Marek Siarkowicz <siarkowicz@google.com>
Copy link
Member

@fuweid fuweid left a comment

Choose a reason for hiding this comment

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

LGTM

@ahrtr
Copy link
Member

ahrtr commented Jun 1, 2023

Overall looks good to me with a minor comment #15987 (comment).

Thanks @tao12345666333 and @jmhbnz

@jmhbnz
Copy link
Member

jmhbnz commented Jun 4, 2023

Hey @ahrtr - Are we ok to merge this now?

Noting we have a decision to make under #15987 about where the config files are going to live, however it would be nice to get this makefile fix functionality merged and close down #15955.

@ahrtr
Copy link
Member

ahrtr commented Jun 4, 2023

Just as I mentioned in #15987 (comment), I don't think it's good to put the config files under hack. The utilities & scripts under hack are various hacks which are used by developers (just for developers reference); while all tools under tools are used by etcd itself. Removing anything under tools will break the etcd (e.g. build, workflow check etc), but removing anything under hack will not impact etcd itself at all.

@jmhbnz
Copy link
Member

jmhbnz commented Jun 4, 2023

Just as I mentioned in #15987 (comment), I don't think it's good to put the config files under hack.

Thanks for sharing your perspective re hack, note this pr does not propose anything under that directory.

@tao12345666333
Copy link
Contributor Author

So can we merge this PR by moving the config files currently in the tools root directory to the tools/config directory?

I personally don't have a strong preference for these two positions.

@ahrtr
Copy link
Member

ahrtr commented Jun 5, 2023

I am OK as long as the config files are under tools instead of hack, thanks.

@ahrtr ahrtr merged commit 8cb3fab into etcd-io:main Jun 5, 2023
@tao12345666333 tao12345666333 deleted the yamlfmt branch June 5, 2023 01:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants