Close outdated PRs before adding new ones

rajatjindal / krew-release-bot

bot to bump version of plugin in krew-index on new releases

Apache License 2.0

46 stars 17 forks source link

Close outdated PRs before adding new ones #21

Closed rikatz closed 3 years ago

rikatz commented 4 years ago

Signed-off-by: Ricardo Pchevuzinske Katz ricardo.katz@gmail.com

This PR is to Close outdated PRs that might be stuck before opening new ones

I didn't tested it in prod, as I actually don't know how to trigger an action to check if its working

Fixes: #19

rajatjindal commented 4 years ago

thank you so very much for opening the PR. I will test it out myself once, and publish a new release.

rikatz commented 4 years ago

@rajatjindal sorry for the long delay.

So I've made the following:

Submit the new PR and return also the number of this PR
With this new PR number check if there are other PRs from the same author, with the same plugin that aren't the same ID of this new one
If this is the case will issue a /close comment referencing the new PR

Let's see if this works :)

ahmetb commented 4 years ago

I think in general features like these create additional complexity. Unless it works super cleanly, I’d normally be against it.

corneliusweig commented 4 years ago

@ahmetb Let's give it a try. Otherwise, the krew maintainers are the only ones who are allowed to close the PR by the bot.

ahmetb commented 4 years ago

I think that’s fine. We always do that anyway.

rajatjindal commented 4 years ago

@ahmetb just to make sure we are on same page, are u recommending we do not pursue this PR? I will go ahead with what you/Cornelius agree upon.

ahmetb commented 4 years ago

I think the search query isn't solid and relies on GitHub's search (which admittedly is pretty bad and isn't an API to be relied on). You might accidentally end up closing other PRs unintended.

If you want to merge, that's fine with me. It'll come at a maintenance cost.

corneliusweig commented 4 years ago

Another, maybe simpler, approach is this:

When creating the PR, always force-push to the same branch in krew-release-bot fork.
Look for an open PR in krew-index by krew-release-bot
- if not present, create a PR
- if present, nothing needs to be done, because the PR was already updated.

That way, there is no need to close old PRs at all. WDYT?

rajatjindal commented 4 years ago

I think this is straight forward enough for now. and only author can close the PR, so it won't close the other PR's.

and also because its server side, if needed, we can push a fix for any issue discovered at a later stage.

thanks Rajat Jindal

rikatz commented 4 years ago

@rajatjindal sorry, I've been digging into other stuff and completely forgot this PR.

Do you still want me to move forward with this feature? There's a final change requested to make a warning instead of an error when not being able to close outdated PRs, I can do this easily :) Just let me know.

Thank you!

rajatjindal commented 4 years ago

Yes please. Its been waiting for you. Thank you for following up

On Thu, Apr 2, 2020 at 7:30 PM Ricardo Katz notifications@github.com wrote:

@rajatjindal https://github.com/rajatjindal sorry, I've been digging into other stuff and completely forgot this PR.

Do you still want me to move forward with this feature? There's a final change requested to make a warning instead of an error when not being able to close outdated PRs, I can do this easily :) Just let me know.

Thank you!

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/rajatjindal/krew-release-bot/pull/21#issuecomment-607864678, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEVN7HYK7JLMICT7BSGU2DRKSLBDANCNFSM4KJKT3FA .

-- Rajat Jindal | Pune | https://rajatjindal.com | rajatjindal83@gmail.com

rikatz commented 4 years ago

@rajatjindal so I've made the last change :) please let me know if there's anything else I can do.

Thank you!