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

add-editor-config #509

Closed
wants to merge 3 commits into from
Closed

Conversation

QuentinBisson
Copy link
Contributor

This PR

  • adds an editorconfig file. For vscode users, you will need to install the editorconfig plugin

Checklist

  • Update changelog in CHANGELOG.md in an end-user friendly language.

Signed-off-by: QuentinBisson <[email protected]>
@QuentinBisson
Copy link
Contributor Author

@TheoBrigitte or @hervenicol could I have some eyes on the shellcheck fixes?

@hervenicol
Copy link
Contributor

Wow....

So, it's been decided that your editor could tamper grafana-exported dashboards at commit? I guess it's towards #502 .
And it also runs shellcheck?
And you updated all the shell scripts that had shellscript alerts?

Most of these changes look legit, some will slightly change the script's behaviour but it looks like it was probably broken before.
I trust you for testing these all.

@marians
Copy link
Member

marians commented Apr 16, 2024

My understanding is that this PR is adding the editorconfig and pre-commit action but it's not enforcing a certain JSON style in CI, correct?

Copy link
Member

@marians marians left a comment

Choose a reason for hiding this comment

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

I think the change on helm/dashboards/values.schema.json is problematic, as these files are usually generated using helm-schema-gen and that tool has its own indentation setting of 4 spaces. So updating the schema will become more painful.

If possible I would exclude this particular file and apply a different setting on it.

@QuentinBisson
Copy link
Contributor Author

@hervenicol no this has not been decided at all :) we can discuss this 😅

@QuentinBisson QuentinBisson force-pushed the add-editor-config-for-formatting branch from 5720304 to 688e56b Compare April 16, 2024 11:15
@QuentinBisson
Copy link
Contributor Author

I removed shellcheck changes to keep the conversation to the main topic which is json formatting

@QuentinBisson QuentinBisson force-pushed the add-editor-config-for-formatting branch 7 times, most recently from 5124131 to c04c77b Compare April 16, 2024 11:42
@QuentinBisson QuentinBisson force-pushed the add-editor-config-for-formatting branch from c04c77b to e0c7da7 Compare April 16, 2024 11:42
@QuentinBisson
Copy link
Contributor Author

Team is not in favor of this, closing

@QuentinBisson QuentinBisson deleted the add-editor-config-for-formatting branch May 29, 2024 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants