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

feat(github): Add the possibility to link a Milestone #27343

Merged
merged 19 commits into from
Feb 21, 2024

Conversation

nils-a
Copy link
Contributor

@nils-a nils-a commented Feb 15, 2024

Changes

New configuration option: milestone.

This option is only available for the GitHub platform.
If set to the title of an existing milestone, the corresponding milestone is found and the PR is added to that milestone.
If a value is provided, but no corresponding milestone can be found, a warning is emitted to the log.

Context

#18372

Documentation (please check one with an [x])

  • I have updated the documentation, or
  • No documentation update is required

How I've tested my work (please select one)

I have verified these changes via:

  • Code inspection only, or
  • Newly added/modified unit tests, or
  • No unit tests but ran on a real repository, or
  • Both unit tests + ran on a real repository

@nils-a
Copy link
Contributor Author

nils-a commented Feb 15, 2024

Does anyone have an idea for a better unit test? I feel the test I provided is not very "resilient" by expecting to get three api calls in the "correct" order.

@nils-a nils-a marked this pull request as ready for review February 15, 2024 22:29
@nils-a nils-a marked this pull request as draft February 15, 2024 22:33
to expect the log.warn
@nils-a nils-a marked this pull request as ready for review February 15, 2024 23:36
@nils-a
Copy link
Contributor Author

nils-a commented Feb 15, 2024

At least GitLab, Gitea and Bitbucket seem to have the notions of milestones, too. Should I try to tackle them in this PR, too?

(I feel it would be "cleaner" to have single issues and PRs for each provider that should support the "milestone" option.)

lib/modules/platform/github/index.ts Outdated Show resolved Hide resolved
lib/modules/platform/github/index.ts Outdated Show resolved Hide resolved
lib/modules/platform/github/index.ts Outdated Show resolved Hide resolved
@viceice
Copy link
Member

viceice commented Feb 16, 2024

At least GitLab, Gitea and Bitbucket seem to have the notions of milestones, too. Should I try to tackle them in this PR, too?

(I feel it would be "cleaner" to have single issues and PRs for each provider that should support the "milestone" option.)

Can be done in seperate PR's

lib/modules/platform/github/index.ts Outdated Show resolved Hide resolved
lib/modules/platform/github/types.ts Outdated Show resolved Hide resolved
docs/usage/configuration-options.md Outdated Show resolved Hide resolved
nils-a and others added 6 commits February 16, 2024 20:04
better options documentation

Co-authored-by: Michael Kriese <michael.kriese@visualon.de>
removed the unused param 'issueNo'
removed the throw exception and logged a warning instead
moved the adding of the milestone to before automerging
defined the milestone type using zod
Copy link
Collaborator

@rarkins rarkins left a comment

Choose a reason for hiding this comment

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

What if we asked for the milestone number instead of title?

lib/modules/platform/github/index.ts Outdated Show resolved Hide resolved
lib/modules/platform/github/index.ts Outdated Show resolved Hide resolved
@nils-a
Copy link
Contributor Author

nils-a commented Feb 17, 2024

That would remove the need to load and cache all milestones.

Should we, in that case, add an API call to check if the milestone exists and is open?

If we had the number, we could also try to set the milestone on the PR (without checking first) and simply log, if that fails.

@rarkins
Copy link
Collaborator

rarkins commented Feb 17, 2024

I'd prefer the happy path to be try setting it and log a warning if it fails

@nils-a nils-a requested a review from rarkins February 18, 2024 21:03
@nils-a nils-a requested a review from viceice February 18, 2024 21:03
rarkins
rarkins previously approved these changes Feb 19, 2024
lib/config/options/index.ts Outdated Show resolved Hide resolved
lib/config/options/index.ts Show resolved Hide resolved
nils-a and others added 2 commits February 19, 2024 14:50
Co-authored-by: HonkingGoose <34918129+HonkingGoose@users.noreply.github.com>
Co-authored-by: HonkingGoose <34918129+HonkingGoose@users.noreply.github.com>
rarkins
rarkins previously approved these changes Feb 19, 2024
Co-authored-by: HonkingGoose <34918129+HonkingGoose@users.noreply.github.com>
Copy link
Collaborator

@HonkingGoose HonkingGoose left a comment

Choose a reason for hiding this comment

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

One more thing, and then the docs are good to go.

docs/usage/configuration-options.md Outdated Show resolved Hide resolved
docs/usage/configuration-options.md Outdated Show resolved Hide resolved
nils-a and others added 2 commits February 21, 2024 13:10
Co-authored-by: HonkingGoose <34918129+HonkingGoose@users.noreply.github.com>
Co-authored-by: HonkingGoose <34918129+HonkingGoose@users.noreply.github.com>
@rarkins rarkins added this pull request to the merge queue Feb 21, 2024
@rarkins rarkins removed this pull request from the merge queue due to a manual request Feb 21, 2024
Copy link
Collaborator

@HonkingGoose HonkingGoose left a comment

Choose a reason for hiding this comment

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

I'm happy with the docs now! 🥳

@rarkins rarkins added this pull request to the merge queue Feb 21, 2024
{
name: 'milestone',
description: `The number of a milestone. If set, the milestone will be set when Renovate creates the PR.`,
type: 'integer',
Copy link
Member

Choose a reason for hiding this comment

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

Why this was changed to integer this would make it probably hard to implemet on other platforms which don't use int as primary key.

it's also much harder to remember the id instead of the title-

@viceice
Copy link
Member

viceice commented Feb 21, 2024

That would remove the need to load and cache all milestones.

Should we, in that case, add an API call to check if the milestone exists and is open?

If we had the number, we could also try to set the milestone on the PR (without checking first) and simply log, if that fails.

I'm not happy with the switch to number 😞 . I know we don't need an additional lookup, but configuration is much harder.

Merged via the queue into renovatebot:main with commit 16589bf Feb 21, 2024
35 checks passed
@renovate-release
Copy link
Collaborator

🎉 This PR is included in version 37.204.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@nils-a
Copy link
Contributor Author

nils-a commented Feb 21, 2024

I'm not happy with the switch to number 😞 . I know we don't need an additional lookup, but configuration is much harder.

I agree, the configuration was much easier with the "title" of the milestone. However, having to load all the open milestones and find the one that matches the title didn't even feel good while typing it.

I am, however, totally prepared to add an alternative configuration option with the title if that is desired.

@nils-a nils-a deleted the feature/GH-18372 branch February 21, 2024 21:45
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants