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

cleaning .gitignore #740

Merged
merged 1 commit into from Nov 18, 2021
Merged

cleaning .gitignore #740

merged 1 commit into from Nov 18, 2021

Conversation

kuritka
Copy link
Collaborator

@kuritka kuritka commented Nov 15, 2021

please try the local branch to see if everything works as expected.
Because things in .gitignore were repetitive and didn't make much sense, I refactored .gitignore.

  • We stayed in denyList mode.
  • It has two sections
    • the first section is generated by toptal.com for vim,go,goland,visualstudiocode,emacs,jekyll
    • the second section follows the first one and is for custom rules of our project.

Signed-off-by: kuritka kuritka@gmail.com

.gitignore Show resolved Hide resolved
.gitignore Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
@kuritka
Copy link
Collaborator Author

kuritka commented Nov 16, 2021

Good points @jkremser, gitignore in this case is generated by https://www.toptal.com/developers/gitignore (which was partly last time too). Only the end of the file (custom section) contains custom rules.

@k0da
Copy link
Collaborator

k0da commented Nov 16, 2021

just for curioucity, is it just IDE runs git add . when handling git? Never had a usecase for git ignore except goreleaser when it requires clean state to perform a release (but IIRC this only relates to tracked files)

Copy link
Contributor

@somaritane somaritane left a comment

Choose a reason for hiding this comment

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

@kuritka I think it makes sense to follow the YAGNI (You Ain't Gonna Need It) principle here and leave only those filters that are relevant to the k8gb project nature and the tooling it uses.
So I would not add auto-generated filters at all, but instead, even further remove unnecessary ones, if we talk about .gitignore cleaning.

Copy link
Member

@ytsarev ytsarev left a comment

Choose a reason for hiding this comment

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

I will second @somaritane here. Let's put only project-related entries into .gitignore

@kuritka
Copy link
Collaborator Author

kuritka commented Nov 18, 2021

.gitignore optimized

chart/k8gb/charts/*.tgz

# Ignore testing kubeconfigs
kubeconfig*
Copy link
Member

Choose a reason for hiding this comment

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

Can we please leave it around? I'm using it for testing over multiple clusters from time to time avoiding tampering the main kubeconfig

Copy link
Contributor

Choose a reason for hiding this comment

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

@ytsarev This looks like a user-specific, not project-specific thing :) i.e. you can export KUBECONFIG= to point to any kubeconfig path of your like, shouldn't be necessary lying in the k8gb working copy. Or am I missing something?

Copy link
Member

@ytsarev ytsarev Nov 18, 2021

Choose a reason for hiding this comment

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

@somaritane looks like project creator specific thing yes ;) it is just two lines to speed up my productivity, I am not asking for too much, I think :)
Re kubeconfigs - I prefer to temporary keep them to the project under test, the matter of convenience

Copy link
Member

Choose a reason for hiding this comment

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

@somaritane following the same logic, you could also use vim and we could drop .vscode and .idea ;)

Copy link
Member

Choose a reason for hiding this comment

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

Also kubeconfigs are somehow related to k8s operator project :D Sorry, now I will not give it up so easily :D

Copy link
Member

Choose a reason for hiding this comment

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

@somaritane works for me, i will sacrifice my kubeconfigs for that :D

Copy link
Collaborator Author

@kuritka kuritka Nov 18, 2021

Choose a reason for hiding this comment

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

I am also in favor of removing IDE specific settings, I left it there more because for most contributors this is too confusing.

Copy link
Member

Choose a reason for hiding this comment

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

pls keep .idea & .vscode also there, I have both in my global .gitignore, but still if it's not there it makes contributing (for external ppl w/ random editors) harder. The chances of using either vs code or idea are pretty high these days

I don't see anything wrong w/ putting ide specific one-liners for IGNORING the configs (like .idea) what seemed weird to me were those !.vscode/tasks.json "keep-ide-specific-stuff-in-repo" rules

btw. I had to add those two specific rules to my .gitignore to be able to work comfortly w/ k8gb:

...
/.custom-env/*               ... custom env files that can be consumed by debug/run configurations in goland
/k8gb                        ... binary file we produce when doing `go build`

Copy link
Collaborator Author

@kuritka kuritka Nov 18, 2021

Choose a reason for hiding this comment

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

just in case, if we leave the DenyListing .gitignore and switch to AllowListing .gitignore instead, we will only allow what is actually part of the project.

Copy link
Contributor

Choose a reason for hiding this comment

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

The chances of using either vs code or idea are pretty high these days
@jkremser This statement is subjective without stats :).

I just hope that If we leave .gitignore for vim, idea, vscode, then we're ready to accept PRs to add emacs, sublime, atom, eclipse, komodo etc (sits pretty well in random editor category with Golang support available) on request in the future, denying such requests would be unfair.

But as I said, if I have my IDE of choice, asking every single project to include it in its .gitignore won't scale.

.gitignore Outdated Show resolved Hide resolved
.gitignore Show resolved Hide resolved
.gitignore Show resolved Hide resolved
@kuritka kuritka force-pushed the clean-gitignore branch 3 times, most recently from b0c2753 to 5bb9d47 Compare November 18, 2021 11:16
please try the local branch to see if everything works as expected.
Because things in .gitignore were repetitive and didn't make much sense, I refactored .gitignore.

- We stayed in denyList mode.
- ~~It has two sections~~
  - ~~the first section is generated by [toptal.com](https://www.toptal.com/developers/gitignore) for vim,go,goland,visualstudiocode,emacs,jekyll~~
  - ~~the second section follows the first one and is for custom rules of our project.~~

Signed-off-by: kuritka <kuritka@gmail.com>
Copy link
Member

@ytsarev ytsarev left a comment

Choose a reason for hiding this comment

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

image
Looks like good cleanup. LGTM

.*.sw[a-p]
# session
*.sw[ap]
k8gb
Copy link
Member

Choose a reason for hiding this comment

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

❤️

Copy link
Contributor

@somaritane somaritane left a comment

Choose a reason for hiding this comment

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

btw. I had to add those two specific rules to my .gitignore to be able to work comfortly w/ k8gb:

@kuritka I think we should add k8gb to .gitignore because this is project-specific thing.

Copy link
Contributor

@somaritane somaritane left a comment

Choose a reason for hiding this comment

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

@kuritka sorry, didn't see k8gb binary is already included.
Thanks a lot for the tremendous effort, LGTM! 👍

@kuritka kuritka merged commit e036573 into master Nov 18, 2021
@kuritka kuritka deleted the clean-gitignore branch November 18, 2021 14:06
jkremser pushed a commit to jkremser/k8gb that referenced this pull request Nov 18, 2021
please try the local branch to see if everything works as expected.
Because things in .gitignore were repetitive and didn't make much sense, I refactored .gitignore.

- We stayed in denyList mode.
- ~~It has two sections~~
  - ~~the first section is generated by [toptal.com](https://www.toptal.com/developers/gitignore) for vim,go,goland,visualstudiocode,emacs,jekyll~~
  - ~~the second section follows the first one and is for custom rules of our project.~~

Signed-off-by: kuritka <kuritka@gmail.com>
Signed-off-by: Jirka Kremser <jiri.kremser@gmail.com>
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.

None yet

5 participants