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

bottle: author bottle commit as BrewTestBot #7246

Merged
merged 1 commit into from Apr 1, 2020

Conversation

dawidd6
Copy link
Member

@dawidd6 dawidd6 commented Mar 31, 2020

As it is in test-bot:
https://github.com/Homebrew/homebrew-test-bot/blob/6b2a8182f9d515f80722fda1dadab43ef4334e01/lib/test_bot.rb#L348-L350

Don't know if it is the correct implementation here, but a move must be made, because currently bottle commits are authored by the committer, rather than @BrewTestBot as it should be:
Homebrew/homebrew-core@242aab6

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew tests with your changes locally?

@dawidd6
Copy link
Member Author

dawidd6 commented Mar 31, 2020

Maybe just set the --author option in git commit here:

safe_system "git", "commit", "--no-edit", "--verbose",
"--message=#{short_name}: #{update_or_add} #{pkg_version} bottle.",
"--", path

@MikeMcQuaid
Copy link
Member

Maybe just set the --author option in git commit here:

Yeh, this needs to happen on the original commit and set the author rather than the committer (which is what this PR is doing).

@dawidd6 dawidd6 changed the title pr-pull: author bottle commit as BrewTestBot bottle: author bottle commit as BrewTestBot Mar 31, 2020
@MikeMcQuaid MikeMcQuaid removed their request for review March 31, 2020 16:37
@dawidd6 dawidd6 requested review from jonchang and removed request for jonchang April 1, 2020 13:39
Copy link
Contributor

@jonchang jonchang left a comment

Choose a reason for hiding this comment

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

Yeah, this looks like a good change.

@dawidd6 dawidd6 merged commit fe6cf57 into Homebrew:master Apr 1, 2020
@dawidd6 dawidd6 deleted the pr-pull-commit-author branch April 1, 2020 15:16
@@ -561,7 +561,8 @@ def merge
pkg_version = bottle_hash["formula"]["pkg_version"]

path.parent.cd do
safe_system "git", "commit", "--no-edit", "--verbose",
author = "BrewTestBot <homebrew-test-bot@lists.sfconservancy.org>"
safe_system "git", "commit", "--no-edit", "--verbose", "--author=#{author}",
Copy link
Member

Choose a reason for hiding this comment

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

This isn't correct. See the Utils.set_git_name_email! which sets these values based on HOMEBREW_GIT_NAME which brew test-bot sets: https://github.com/Homebrew/homebrew-test-bot/blob/355af90a55c07e079009a7862c137d65ee8db75c/lib/test_bot.rb#L348-L350.

As-is this will now override these values and set the author to @BrewTestBot for all uses of brew bottle --commit e.g. non-official taps.

Copy link
Member

Choose a reason for hiding this comment

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

This seems like an Actions-specific bug.

Copy link
Member Author

Choose a reason for hiding this comment

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

But even if maintainer would run brew pr-pull locally on their machine, the author of the bottle commit will still not be @BrewTestBot.

Copy link
Member

Choose a reason for hiding this comment

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

HOMEBREW_GITHUB_NAME etc. should be set in brew pr-pull for when it calls brew bottle in that case. If the bottle commit is being created locally, though, I'm not sure why it's desirable to have it attributed to @BrewTestBot anyway?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just to keep things as they are right now, I guess. If it's not required, then okay.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry 😭

Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably set those variables in the homebrew/core pr-publish workflow if we haven't already. I think we have but I need to check that it works properly.

Copy link
Member

Choose a reason for hiding this comment

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

Are we pulling those commits or are we creating the bottle commits locally?

Copy link
Member Author

Choose a reason for hiding this comment

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

Creating locally.

Copy link
Member

Choose a reason for hiding this comment

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

To be that sounds a bit like we should be attributing them to the local user, yeh.

@lock lock bot added the outdated PR was locked due to age label May 5, 2020
@lock lock bot locked as resolved and limited conversation to collaborators May 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants