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

perf: enable renovate to set git author when using github app token #436

Closed
wants to merge 2 commits into from

Conversation

msclock
Copy link
Contributor

@msclock msclock commented Mar 26, 2024

Refer to https://docs.renovatebot.com/modules/platform/github/#running-as-a-github-app, when using self-host renovate on github, we only need to set the RENOVATE_USERNAME.

Related #400

@msclock msclock force-pushed the opt_renovate_job branch 2 times, most recently from 0a713f8 to 2e6ccdc Compare March 26, 2024 03:46
@msclock msclock requested a review from huxuan March 26, 2024 06:01
@msclock msclock marked this pull request as draft March 26, 2024 06:06
@msclock
Copy link
Contributor Author

msclock commented Mar 26, 2024

WIP and pending to check GITHUB_TOKEN permissions.

run: |
if [ -n "${{ steps.generate-token.outputs.token }}" ]; then
Copy link
Member

Choose a reason for hiding this comment

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

I have several concern about the logic here:

  1. According to the doc, username is also optional, autodetected if not supplied, have you ever tried whether that works?
  2. If the template users are using PAT and no variable RNOVATE_GIT_AUTHOR is provided, we should use the default value, but seems the default value is removed.
  3. If the template users provide a specific value to variable RENOVATE_GIT_AUTHOR, we should respect it whenever GitHub App is used.

Copy link
Contributor Author

@msclock msclock Mar 26, 2024

Choose a reason for hiding this comment

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

It needs to further verify feasibility on it.

Copy link
Contributor Author

@msclock msclock Apr 16, 2024

Choose a reason for hiding this comment

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

I have verified the following scenes:

  • only bot token(not username): it detects username and sets the correct bot account.
  • bot token and git author: it still resolves to use bot account.
  • bot token and not git author(use yaml default author explictly): use bot account again.

I don't known how it creates a ghost user now and feel confused.🤣

Copy link
Member

Choose a reason for hiding this comment

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

Is there any corresponding job logs or created pull requests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

from actions #3, commit renovate job run manually log says it use configured git author. But the created pr #1 #2 both are authored by my configured bot account.

Copy link
Member

Choose a reason for hiding this comment

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

From my understanding, they are two different things. The author of the pull request author is only determined by the token used when creating the pull request. So it will either be a personal account when using PAT or bot account when using token generated by GitHub App. The renovate configuration only affects the git author of the commits, as you can see in the commits tab [1][2].

[1] https://github.com/msclock/smooth7zip/pull/1/commits
[2] https://github.com/msclock/smooth7zip/pull/2/commits

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@huxuan Ah! I missed that flaws after several weeks.🤣

Copy link
Member

Choose a reason for hiding this comment

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

Seems we have consumed too much energy on this, if necessary, we can have a online discussion about this.

Copy link
Contributor Author

@msclock msclock Apr 16, 2024

Choose a reason for hiding this comment

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

@huxuan it's all right. just need to do trade-offs. I have made another pr #470 and switch to just warn users. Feel free to alter it if you need any improvement.

@msclock msclock closed this Apr 16, 2024
@huxuan huxuan deleted the opt_renovate_job branch April 16, 2024 01:47
@huxuan huxuan restored the opt_renovate_job branch April 16, 2024 05:37
@msclock msclock reopened this Apr 16, 2024
@msclock msclock closed this Apr 16, 2024
@msclock msclock deleted the opt_renovate_job branch April 16, 2024 10:15
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

2 participants