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

1254: Add a checkbox for CSR requirement #1246

Closed
wants to merge 3 commits into from

Conversation

lgxbslgx
Copy link
Member

@lgxbslgx lgxbslgx commented Nov 27, 2021

Hi all,

This patch adds a checkbox for CSR requirement in the Progress and adds the corresponding test cases.

Thanks for taking the time to review.

Best Regards,
-- Guoxiong


Progress

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

Issue

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1246

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented Nov 27, 2021

👋 Welcome back gli! 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 added the rfr label Nov 27, 2021
@mlbridge
Copy link

mlbridge bot commented Nov 27, 2021

Webrevs

if (pr.labelNames().contains("csr")) {
ret.put("Change requires a CSR request to be approved", false);
} else {
ret.put("Change doesn't require a CSR request or the CSR request has been approved", true);
Copy link
Member

@erikj79 erikj79 Nov 29, 2021

Choose a reason for hiding this comment

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

I don't think we should show anything here if a CSR was never requested. The difference between "CSR was never requested" and "CSR is approved" isn't really tracked currently, so that would be tricky. I'm guessing this is a big reason why the original implementation didn't add checkboxes, but only a blocker message when active. For this to work we will need to add some state that tracks if a CSR is currently required. We basically have 3 states:

  1. No CSR needed
  2. CSR needed and not yet approved
  3. CSR needed and approved

Every PR starts in state 1. To get to state 2, you can either issue the /csr command, or with SKARA-1071 file a CSR and let Skara find it. To get to state 3, the CSR must be approved. At any time, the command "/csr unneeded" can take you back to state 1.

This could be implemented with another label, or it could rely on special comments with markers. In this case, I think we want to make it visible to users, so labels probably make more sense. For backwards compatibility, to keep the current meaning of the "csr" label, the new label would need to be something like "csr-approved" and be added at the same time the csr label is removed when moving to state 3.

Copy link
Member Author

@lgxbslgx lgxbslgx Nov 29, 2021

Choose a reason for hiding this comment

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

@erikj79 The three states you mentioned above is clear. From your comment, I conclude the following things:

  1. No CSR needed: the PR doesn't have csr and csr-approved labels.
  2. CSR needed and not yet approved: the PR has csr label and doesn't have csr-approved label.
  3. CSR needed and approved: the PR has csr-approved label and doesn't have csr label.

And the csr label and csr-approved label can't appear at the same time.

But currently, the CSRBot and CSRCommand have the following logic:

  1. No CSR needed: the PR doesn't have csr label and the csr issue doesn't exist or has been withdrawn.
  2. CSR needed and not yet approved: the PR has csr label.
  3. CSR needed and approved: the PR doesn't have csr label and the csr issue has been Approved.

If we obey the new logic and create the new label csr-approved, some checks can be achieved easily. But the logic change may cause the unexpected regression. As you can see the discussion in the PR-1245 of the SKARA-1071, both the authors of the patch and the reviewers need to be very careful to avoid the misunderstanding and avoid making a mistake.

So I would like to obey the origin logic. In this patch, I can check the csr link and issue to identify the state 1 and state 3 which is like the CSRBot and CSRCommand. So we don't need to add another label such as csr-approved to solve this problem. What is your opinion about this?


And a related thing:
There are many places which need to get the csr issue from the original issue. We should provide a common method to solve this common problem. So I filed SKARA-1256 to solve it.

Copy link
Member

@erikj79 erikj79 Nov 29, 2021

Choose a reason for hiding this comment

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

I needed to think about this for a bit, but I think you are right.

I was instinctively trying to keep the PR bot from having to query JBS for CSR status, because we currently have a label which made checking the CSR state very cheap and obvious. But on the other hand, we are fetching the issue from JBS anyway during every CheckRun, so adding this extra step isn't that expensive.

Copy link
Member Author

@lgxbslgx lgxbslgx Nov 30, 2021

Choose a reason for hiding this comment

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

@erikj79 I re-think about this. Even though we decide to use the new logic and add the new label csr-approved at the end, we shouldn't implement the feature at this patch. Because the goal of this patch is only to add a checkbox for CSR requirement.

So I advice to file a new issue to continue the discussion. The new issue title can be Investigating to add a label 'csr-approved' to mark the CSR is approved.

Back to this patch, I think it is good to use the old logic to implement the enhancement so that the users (OpenJDK developers) can take advantage of this patch as soon as possible.

Copy link
Member

@erikj79 erikj79 Nov 30, 2021

Choose a reason for hiding this comment

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

I don't think we need a new label. Your suggestion to tell state 1 and 3 apart by checking if a CSR issue exists seems good enough to me.

@lgxbslgx
Copy link
Member Author

lgxbslgx commented Dec 1, 2021

The code was updated. Thanks for the review.

And I find an intermittent issue. Filed SKARA-1264.

magicus
magicus approved these changes Dec 1, 2021
Copy link
Member

@magicus magicus left a comment

This looks good to me, now.

@openjdk
Copy link

openjdk bot commented Dec 1, 2021

@lgxbslgx 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.

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

1254: Add a checkbox for CSR requirement

Reviewed-by: ihse, erikj

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 no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@magicus, @erikj79) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready label Dec 1, 2021
erikj79
erikj79 approved these changes Dec 1, 2021
Copy link
Member

@erikj79 erikj79 left a comment

Looks good!

@lgxbslgx
Copy link
Member Author

lgxbslgx commented Dec 1, 2021

@erikj79 @magicus Thanks for the reviews. Could I get your help to sponsor this patch?

/integrate

@openjdk openjdk bot added the sponsor label Dec 1, 2021
@openjdk
Copy link

openjdk bot commented Dec 1, 2021

@lgxbslgx
Your change (at version fab5212) is now ready to be sponsored by a Committer.

@erikj79
Copy link
Member

erikj79 commented Dec 1, 2021

/sponsor

@openjdk
Copy link

openjdk bot commented Dec 1, 2021

Going to push as commit 08548eb.

@openjdk openjdk bot closed this Dec 1, 2021
@openjdk
Copy link

openjdk bot commented Dec 1, 2021

@erikj79 @lgxbslgx Pushed as commit 08548eb.

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

@lgxbslgx lgxbslgx deleted the SKARA-1254-NEW branch Dec 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants