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

Create GitHub action to put current milestone on issue of closed PR #12497

Closed
romani opened this issue Dec 3, 2022 · 27 comments · Fixed by #12782 or #12821
Closed

Create GitHub action to put current milestone on issue of closed PR #12497

romani opened this issue Dec 3, 2022 · 27 comments · Fixed by #12782 or #12821

Comments

@romani
Copy link
Member

romani commented Dec 3, 2022

We put milestone manually after we merge PR.

This should be automatic.
https://www.jessesquires.com/blog/2022/08/04/automatically-assign-milestones-with-github-actions/
https://github.com/orgs/community/discussions/26724#discussioncomment-3253107

Or something special for us. As sometimes pull request needs to marked with milestone as it is referred in commit, but it is rare case we can skip it if this is complicated.

@romani romani added the approved label Dec 3, 2022
@rnveach
Copy link
Member

rnveach commented Dec 3, 2022

It can't be automatic if the issue already has a milestone set. We sometimes have issues that span multiple releases.

@romani
Copy link
Member Author

romani commented Dec 3, 2022

I think it is fine to have latest milestone. Long lasting usually misc issues, user unlikely interested in watching them.

@ayush27prasad
Copy link

ayush27prasad commented Jan 25, 2023

@romani I am working on this issue, could you please assign it to me?

Also, I have a query as the current milestone is 103 so if any PR is merged, it is assigned to this milestone only or the latest milestone be created in future?

@romani
Copy link
Member Author

romani commented Jan 26, 2023

We are not doing assignments, just make comment "I am on it" and got it.

Yes, the are only single milestone active all the time, there are no time when we have two. And all merged should be marked by such single milestone milestone.

@romani
Copy link
Member Author

romani commented Feb 4, 2023

@Kevin222004, can you help with this issue?

@Kevin222004
Copy link
Contributor

I am on it

@stoyanK7
Copy link
Contributor

@Kevin222004 Are you still working on it? I am interested to take it.

@Kevin222004
Copy link
Contributor

@stoyanK7 please take it I am busy with exam till 2march
And then still I have to learn more about this.

Please work on it

@stoyanK7
Copy link
Contributor

Thanks a lot.

On it then.

@rnveach
Copy link
Member

rnveach commented Mar 4, 2023

Not working at #12797 (comment)
Please note task did not fail. We should fail on any error.

@rnveach rnveach reopened this Mar 4, 2023
@romani
Copy link
Member Author

romani commented Mar 5, 2023

@stoyanK7, please take a look

@stoyanK7
Copy link
Contributor

stoyanK7 commented Mar 5, 2023

@rnveach

We should fail on any error.

From what I remember, --fail-with-body was not available in all images so I didn't even bother adding it: #12451
I will try adding it to see if it is supported now.


https://github.com/checkstyle/checkstyle/actions/runs/4332960192/jobs/7565786160
{
"message": "Resource not accessible by integration",
"documentation_url": "https://docs.github.com/rest/reference/issues#update-an-issue"
}

It seems that the GITHUB_TOKEN does not have sufficient permissions to update the issue milestone which is strange.
We should have the right permissions for this:

permissions:
issues: write
pull-requests: write

However, at the Set up job step of the failing job, this is shown:
https://github.com/checkstyle/checkstyle/actions/runs/4332960192/jobs/7565786160#step:1:17

GITHUB_TOKEN Permissions
  Issues: read
  Metadata: read
  PullRequests: read

In my fork runs the permissions are set right:
https://github.com/stoyanK7/checkstyle/actions/runs/4315749586/jobs/7530566547#step:1:16

GITHUB_TOKEN Permissions
  Issues: write
  Metadata: read
  PullRequests: write

Could it be some repository setting or..?

@stoyanK7
Copy link
Contributor

stoyanK7 commented Mar 5, 2023

@rnveach
Copy link
Member

rnveach commented Mar 5, 2023

@romani Ping. I have no knowledge here.

@romani
Copy link
Member Author

romani commented Mar 5, 2023

this is might be token or permission that author of PR has or specially limited to avoid outside people to get access to resources.
in Fork all is done by owner, trusted person.

as we have special logic in action ${{ github.event.pull_request.merged == true }} that limits execution a lot, and content is trusty in this case. We should use PAT.

all existing actions for release, executed by maintainers, trusty accounts.

@rnveach
Copy link
Member

rnveach commented Mar 5, 2023

@romani @stoyanK7
https://github.com/checkstyle/checkstyle/actions/workflows/set-milestone-on-referenced-issue.yml
https://github.com/checkstyle/checkstyle/actions/runs/4336909275/jobs/7572544051

Run ./.ci/set-milestone-on-referenced-issue.sh 12799
PULL_REQUEST_NUMBER=12799
Error: Set not empty value to GITHUB_TOKEN environment variable
Error: Process completed with exit code 1.

This looks to be the latest from what I saw. It looks like this runs on the same PR as it was merged from.

@stoyanK7
Copy link
Contributor

stoyanK7 commented Mar 5, 2023

@rnveach @romani Is there a PAT secret in the repository secrets? There must be, since it is used in release workflows.

@stoyanK7
Copy link
Contributor

stoyanK7 commented Mar 5, 2023

I think the issue comes from the

on:
  pull_request:
    types:
      - closed

Something tells me action should be executed on pushes to master.

@romani
Copy link
Member Author

romani commented Mar 5, 2023

Something tells me action should be executed on pushes to master.

it will be more secure, and probably logical, as we use commit message as source of truth, it does not matter how commit come to master branch, it will be part of next release. The only extra complexity is what range of commits to use for validation.
May be deactivated concurrency and auto cancel will do a job for this action.

@stoyanK7
Copy link
Contributor

stoyanK7 commented Mar 5, 2023

@romani

The only extra complexity is what range of commits to use for validation.

Could you explain more?

For sure concurrency has to be deactivated.

auto cancel will do a job for this action.

What do you mean by auto cancel?

@romani
Copy link
Member Author

romani commented Mar 5, 2023

There are cases when one PR is merging two commits, we need to make sure a action will run on both commits.

What do you mean by auto cancel?

cancel-in-progress: true

@stoyanK7
Copy link
Contributor

stoyanK7 commented Mar 5, 2023

Oh yeah, makes sense. Back on it.

@romani
Copy link
Member Author

romani commented Mar 5, 2023

I think we need to repeat what our release notes builder is doing in iteration of commits. Execute in same way.
What builder see for release, that should be marked by milestone.

Maybe even add this functionality to builder as extra mode.

@romani
Copy link
Member Author

romani commented Mar 6, 2023

https://github.com/checkstyle/checkstyle/actions/runs/4339680750/jobs/7577497529

0s
Run ./.ci/set-milestone-on-referenced-issue.sh [1](https://github.com/checkstyle/checkstyle/actions/runs/4339680750/jobs/7577497529#step:3:1)2775
PULL_REQUEST_NUMBER=12775
Error: Set not empty value to GITHUB_TOKEN environment variable
Error: Process completed with exit code 1

@romani
Copy link
Member Author

romani commented Mar 7, 2023

Fixed by #12801

@romani romani closed this as completed Mar 7, 2023
@rnveach
Copy link
Member

rnveach commented Mar 9, 2023

@stoyanK7 This still seems to be broken.
https://github.com/checkstyle/checkstyle/actions/runs/4369202639/jobs/7642723623#step:3:46

Please review and confirm.

@rnveach rnveach reopened this Mar 9, 2023
@romani
Copy link
Member Author

romani commented Mar 9, 2023

Yes, if not issue or pull, we should skip such commit

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