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

fix: another cleanup and linting (contains move to configfiles) #478

Closed
wants to merge 5 commits into from

Conversation

wrenix
Copy link
Collaborator

@wrenix wrenix commented Nov 19, 2023

few parts cleanup and lint

contains commit of:

Checklist

@wrenix wrenix force-pushed the patch-2 branch 5 times, most recently from f2b9eba to 124e563 Compare November 19, 2023 15:30
@wrenix wrenix changed the title fix: cleanup metrics components (and move to folder) fix: another cleanup and linting (contains move to configfiles) Nov 19, 2023
@jessebot
Copy link
Collaborator

jessebot commented Nov 20, 2023

This is too big of a commit to be a patch change. This should be a minor or major change, but I think it might be best to avoid these massive PRs in the future, as they're hard to review. People use this helm chart for important personal and professional aims, and we should try to make PRs as clear and concise as possible, to help others debug any potential bugs that may arise. Please instead target specific and small changes.

@wrenix
Copy link
Collaborator Author

wrenix commented Nov 20, 2023

I understand your position, for the future i will make it - i has already said in my PSS, that i will create smaller PRs.

So what should i do with this PR now.
change the semver level or splitt it up in multiple commits or multiple PRs?

@jessebot
Copy link
Collaborator

Let's do specific small PRs and close this one for now. We still need to test and fix the bug from the other day before doing more clean up though as the chart is currently broken on the latest version. Thank you for understanding

@wrenix wrenix marked this pull request as draft November 20, 2023 10:03
@wrenix wrenix marked this pull request as ready for review November 20, 2023 17:28
@wrenix wrenix force-pushed the patch-2 branch 2 times, most recently from 2f5a084 to f16feec Compare November 22, 2023 22:03
@jessebot
Copy link
Collaborator

I am going to close this in favor of #480 and #481

@jessebot jessebot closed this Nov 24, 2023
@wrenix
Copy link
Collaborator Author

wrenix commented Nov 25, 2023

here are still two commits, with another cleanup

@jessebot
Copy link
Collaborator

jessebot commented Nov 25, 2023

If there are still two smaller commits left on this repo, could you please isolate them and explain what they do in the PR description, and then I can reopen this, or you can option a new PR, up to you.

@wrenix wrenix mentioned this pull request Nov 27, 2023
3 tasks
@wrenix wrenix deleted the patch-2 branch February 5, 2024 11:49
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.

2 participants