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

Unblock after JuliaRegistrator force-pushed? #97

Closed
tkf opened this issue Nov 7, 2019 · 9 comments
Closed

Unblock after JuliaRegistrator force-pushed? #97

tkf opened this issue Nov 7, 2019 · 9 comments

Comments

@tkf
Copy link

tkf commented Nov 7, 2019

In JuliaRegistries/General#5060, it seemed that auto-merge did not kick in even after JuliaRegistrator force-pushed and auto-merge check passed. This is presumably because earlier comments did not include [noblock]. Is it possible to unblock auto-merge in a case like this?

@DilumAluthge
Copy link
Member

DilumAluthge commented Nov 7, 2019

The automerge is blocked by any blocking comments. This is the intended behavior.

For example, suppose I make a registration to register my package Foo version 0.1.1. The previous registered version was 0.1.0.

Suppose that for whatever reason (maybe I don't have compat entries), the automerge fails.

Now, you post a comment saying the following: "Dilum, my package Bar relies on your package Foo. And your version 0.1.1 actually made some breaking changes that broke my package Bar. So you should make a breaking release 0.2.0 instead of a non-breaking change 0.1.1."

Your comment does not include the text [noblock].

I ignore your comment, and I add compat entries to my package, and I re-trigger Registrator, and Registrator force-pushes, and the new auto-merge check passes.

But my PR should not be automerged. You posted a blocking comment!


The solution in your case is to have all commenters edit their comments to include the text [noblock]. Once all comments include the text [noblock], the PR will be automerged.

@tkf
Copy link
Author

tkf commented Nov 7, 2019

I think it would be nice to at least update the comment generated by the bot before closing it.

It is not clear how the bot works if you are not the implementer. (e.g., the bot can have state which is updated when the comment with noblock is made).

Now, you post a comment saying the following

OK I didn't even realize that it was designed to be stoppable by anyone. IMHO, this design choice seems to have some problems:

  • Some casual Julia user can open up an issue and comment Thanks a lot! and then doesn't open GitHub next few months. Remember that people don't read warnings.

  • It is not clear who is the target of this feature. Should package authors watch all the upstream packages?

  • The update frequency is too high for package authors living in different time zones to give a feedback.

@fredrikekre fredrikekre reopened this Nov 7, 2019
@fredrikekre
Copy link
Member

Some casual Julia user can open up an issue and comment Thanks a lot! and then doesn't open GitHub next few months. Remember that people don't read warnings.

Registry maintainers can edit comments (I have done this multiple times for people).

It is not clear who is the target of this feature. Should package authors watch all the upstream packages?

The target is the package author itself mostly, since it is quite common that you realize you forgot something for the release the moment you submitted the registry PR. (It has happened to me multiple times).

The update frequency is too high for package authors living in different time zones to give a feedback.

See above.

However, should we consider only checking comments after the last approve? It is quite easy to forget to update your comment.

@DilumAluthge
Copy link
Member

However, should we consider only checking comments after the last approve? It is quite easy to forget to update your comment.

It kind of defeats the purpose of having blocking comments if you can just invalidate all blocking comments by re-triggering Registrator.

@fredrikekre
Copy link
Member

🤷‍♂️ it is almost always the package author that wants to block, and when that package author has retriggered it is presumable because the blocking comment is not valid anymore?

If we want maintainers to block, maybe look for a DO NOT MERGE label instead?

@DilumAluthge
Copy link
Member

Those are fair points.

@DilumAluthge
Copy link
Member

However, should we consider only checking comments after the last approve? It is quite easy to forget to update your comment.

I won’t oppose this change.

@DilumAluthge
Copy link
Member

Is this still an issue in practice? I think things have been going pretty smoothly with AutoMerge, right?

So I am inclined to close this again as wontfix.

Worst case, a registry maintainer can just edit a comment to add the [noblock] text.

@DilumAluthge
Copy link
Member

Is this still an issue in practice? I think things have been going pretty smoothly with AutoMerge, right?

So I am inclined to close this again as wontfix.

Worst case, a registry maintainer can just edit a comment to add the [noblock] text.

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

No branches or pull requests

3 participants