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

1084: Automatically update repository labels from Skara config #1194

Closed
wants to merge 2 commits into from

Conversation

erikj79
Copy link
Member

@erikj79 erikj79 commented Jul 6, 2021

This patch adds a new WorkItem to the MailingListBridgeBot. The new LabelsUpdaterWorkItem runs once at startup and makes sure all mailing list labels configured for the repository actually exist as labels in the hosted repository (in GitHub or GitLab). I chose to implement this as a WorkItem, even though it's just run once. It will also block any other WorkItems in the bot from running until successful. By using the existing framework for this, we get automatic retries and I believe it will play better with other bots for resource usage, even though it doesn't quite fit in.

The new WorkItem also keeps the description field updated with the mailing list email address. This use of the description field was initially added manually for the main jdk repository on GitHub, but has not been kept up to date since, and has not been propagated to other repositories (jdk16 and jdk17). These description fields are both informative for the user, as well as being used by the CLI client to present information to users.

To verify the new interaction for managing labels on a repository with both GitLab and GitHub, I have implemented a new type of manual tests. They are disabled from automatic running in Gradle using the @disabled annotation. To run them you need to provide a properties file with data about account and keys to use to access the servers. We don't want these kinds of tests to run against GitHub/GitLab automatically, but at least now there is a way to semi automatically verify that kind of functionality.


Progress

  • Change must not contain extraneous whitespace
  • Change must be properly reviewed

Issue

  • SKARA-1084: Automatically update repository labels from Skara config

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/skara pull/1194/head:pull/1194
$ git checkout pull/1194

Update a local copy of the PR:
$ git checkout pull/1194
$ git pull https://git.openjdk.java.net/skara pull/1194/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1194

View PR using the GUI difftool:
$ git pr show -t 1194

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/skara/pull/1194.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jul 6, 2021

👋 Welcome back erikj! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot changed the title SKARA-1084 1084: Automatically update repository labels from Skara config Jul 6, 2021
@openjdk openjdk bot added the rfr label Jul 6, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Jul 6, 2021

Webrevs

Copy link
Member

@kevinrushforth kevinrushforth left a comment

Not a complete review, but I left a couple questions inline.

// Color is Gray and matches all current labels
.put("color", "ededed");
Copy link
Member

@kevinrushforth kevinrushforth Jul 6, 2021

Choose a reason for hiding this comment

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

Will this overwrite the existing color scheme? For a new repo, will it always use gray for all labels?

Copy link
Member Author

@erikj79 erikj79 Jul 7, 2021

Choose a reason for hiding this comment

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

If addLabel() is called on an existing label, then I think the color scheme will be overwritten. The intention not to change any label, just to add the ones that are missing. The updateLabel method will not change the color.

Note that the labels we are adding with this patch are just the ones for mailing lists. Do you think we need to add automation for colors for the other set of labels we use (e.g. rfr, ready, csr etc)?

Copy link
Member

@kevinrushforth kevinrushforth Jul 7, 2021

Choose a reason for hiding this comment

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

I think if we do add automation for colors for the other set of labels, that could be done as a follow-on fix (not sure it's needed though). I mainly wanted to make sure we wouldn't overwrite the ones for rfr, ready, etc. with gray.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

Looks good.

@openjdk
Copy link

@openjdk openjdk bot commented Jul 7, 2021

@erikj79 This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

🔍 One or more changes in this pull request modifies files in areas of the source code that often require two reviewers. Please consider if this is the case for this pull request, and if so, await a second reviewer to approve this pull request before you integrate it.

🌎 Applicable reviewers for one or more changes in this pull request are spread across multiple different time zones. Please consider waiting with integrating this pull request until it has been out for review for at least 24 hours to give all reviewers a chance to review the pull request.

After integration, the commit message for the final commit will be:

1084: Automatically update repository labels from Skara config

Reviewed-by: kcr

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 2 new commits pushed to the master branch:

  • 2540619: 1085: Gitlab PR has bad link to patch file
  • ac074bc: 1095: Ignore tags in pr branches when notifying

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Jul 7, 2021
@erikj79
Copy link
Member Author

@erikj79 erikj79 commented Jul 9, 2021

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Jul 9, 2021

Going to push as commit 6c33395.
Since your change was applied there have been 3 commits pushed to the master branch:

  • 2ae00f8: 1102: Document minimum required JDK version 16 for Skara
  • 2540619: 1085: Gitlab PR has bad link to patch file
  • ac074bc: 1095: Ignore tags in pr branches when notifying

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Jul 9, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Jul 9, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jul 9, 2021

@erikj79 Pushed as commit 6c33395.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants