search

guacsec / guac

GUAC aggregates software security metadata into a high fidelity graph database.
https://guac.sh
Apache License 2.0
1.29k stars 176 forks source link

Disable Kodiak Bot Merge Feature #180

Closed cpendery closed 2 years ago

cpendery commented 2 years ago

Can we please disable the Kodiak Bot merge feature for the following reasons?

  1. The maintainers wants linear history in the PR, so this doesn't meet that minimum requirements
  2. The Kodiak Bot fails the CLA check, causing working PRs to be marked as failing once the bot merges
mlieberman85 commented 2 years ago

Thanks for pointing this out. It looks like it's just misconfigured. It should be configured to not create a merge or squash commits, just to rebase.

lumjjb commented 2 years ago

@rgreinho can you help take a look at this? Thanks.

rgreinho commented 2 years ago

Sure thing!

  1. The bot is configured to squash the PR, because 1 PR must be one commit. Otherwise we end up having a ton non-nonsensical commits cluttering the history ("fix typo", "save", "just trying something", etc.).
  2. I was not able to see the DCA problem. Could you point me to a commit which failed because of the bot?
mlieberman85 commented 2 years ago

for 1, I think we should put the burden on the committer to squash their commits. We don't know how they want to fix them up.

rgreinho commented 2 years ago

I can change it in the configure for sure, but I believe that's too big of a task for the committers in general and it will end up being skipped...

mlieberman85 commented 2 years ago

Personally I just do git commit --amend on any PR I'm working on or rebase -i if I end up wanting to create commits I'm not sure I wanna keep. I just think we can require one single commit. and just block until they've fixed it. I don't want us to make a fix though that ends up causing a bigger annoyance than the squash.

rgreinho commented 2 years ago

I totally agree with you! But sadly, not some many contributors do that 😢

cpendery commented 2 years ago

@rgreinho For reference, my pr just merged with CLA issues. It doesn't cause DCO issues https://github.com/guacsec/guac/pull/150

cpendery commented 2 years ago

@lumjjb I think this issue should be opened back up. The fix merged makes the auto-merge functionality (for merging once the pr is complete) into a rebase rather than a squash. The issue is when the Kodiak bot merges changes into an open PR. It happened a few hours ago as seen above ^

lumjjb commented 2 years ago

i've been looking at this right now, as you may see my commits :p... i think this will fix it : https://github.com/guacsec/guac/pull/187

lumjjb commented 2 years ago

we'll keep it squash for now and this should fix issues with the CLA for now... we will talk more about rebase later on, since its more of an opinion rather than functional.

cpendery commented 2 years ago

Sounds good, it definitely fixed the issue since after merging Kodiak didn't add it to my pr again. Thank you!