jdx / mise

dev tools, env vars, task runner
https://mise.jdx.dev
MIT License
10.23k stars 294 forks source link

java core plugin metadata stuck/out-of-date #2390

Open chadlwilson opened 4 months ago

chadlwilson commented 4 months ago

Describe the bug The metadata actions running at https://github.com/jdx/rtx-java-metadata seem to be failing. There were Java releases earlier this week but they seem to not be flowing through.

https://github.com/jdx/rtx-java-metadata/actions/workflows/update.yml

Root cause seems to be Required status check "test" is expected.. I'm guessing the direct push-to-main action is not able to satisfy this status check?

There are no code changes since then, so I guess either the repository config was changed, or GitHub changed the way these work? (edit: Now I think about it, if there were no Java releases there would have been nothing to do/push on earlier dates so this might have been broken earlier than July 16 - it was just that July 16 was the Java quarterly release date)

Push to branch main
remote: error: GH006: Protected branch update failed for refs/heads/main.        
remote: error: Required status check "test" is expected.        
To https://github.com/jdx/rtx-java-metadata.git
 ! [remote rejected]     HEAD -> main (protected branch hook declined)
error: failed to push some refs to 'https://github.com/jdx/rtx-java-metadata.git'
Error: Invalid exit code: 1
    at ChildProcess.<anonymous> (/home/runner/work/_actions/ad-m/github-push-action/master/start.js:30:21)
    at ChildProcess.emit (node:events:519:28)
    at maybeClose (node:internal/child_process:1105:16)
    at ChildProcess._handle.onexit (node:internal/child_process:305:5) {
  code: 1
}

To Reproduce mise use java@temurin-21.0.4+7.0.LTS should be working by now, all releases for major archs have been published and are on the Adoptium API.

https://api.adoptium.net/v3/assets/feature_releases/21/ga?page=0&page_size=20&project=jdk&sort_order=ASC&vendor=adoptium

Expected behavior mise use java@temurin-21.0.4+7.0.LTS works :-)

Additional context Sorry, issues tab is disabled on that repo, so raising here.

chadlwilson commented 4 months ago

Oh, I see this may be a known issue based on the comment at https://github.com/jdx/rtx-java-metadata/pull/7

roele commented 4 months ago

Yes i assume some branch protection settings are in place on the repository which prevent the action from pushing the changes successfully. Same actions work fine on my fork at https://github.com/roele/mise-java-metadata/actions.

chadlwilson commented 4 months ago

Should be pretty easy to remove; just requires a bit of care reviewing PRs.

Alternatively, change the automation to push to a branch instead + create a PR which is set to auto-merge/squash/rebase+merge on test pass :-)

jdx commented 4 months ago

@roele since my availability is very poor this summer for mise, I went ahead and added you as a collab for the Java Metadata repo to figure out what settings may need to change.

jdx commented 4 months ago

I will note the obvious that given we’re in a state where we can’t even review PRs because the diffs are obvious is not good. I considered maybe this should just be rolled into the version host but then again just the Java metadata alone is way more complex than everything else put together.

roele commented 4 months ago

@jdx Thanks for adding me as collaborator. The permissions do not seem to allow me access to the repository settings though. I suspect that push to main is forbidden/restricted. Maybe the workflow should create a PR which is automerged as suggested by @chadlwilson instead.

Regarding the PRs i share your sentiment. The fact that the each update touches thousands of files is insane. GitHub is not even showing the diff because of the amount of changes.

chadlwilson commented 4 months ago

@jdx Thanks for adding me as collaborator. The permissions do not seem to allow me access to the repository settings though. I suspect that push to main is forbidden/restricted. Maybe the workflow should create a PR which is automerged as suggested by @chadlwilson instead.

Yeah, I think you need to be admin or repo owner to change that. Should be pretty easy to change the rule though:

image image

Just means a bit more discipline when reviewing any PR to ensure the checks are passed before hitting merge, which should be manageable for that repo given the limited contributors and low PR count.

roele commented 4 months ago

@chadlwilson Since the update-metadata workflow runs every 12h we don't want to merge these manually. Manual merges for other/manual PRs affecting the bash scripts etc. would be good though.

I was playing around with this in a personal repository (https://github.com/roele/create-pr-test) using peter-evans/create-pull-request@v6 to create a pull request for the scheduled workflow. There seems to be some pitfalls though like using GITHUB_TOKEN which prevents other related workflows such as a Test to run. Also cannot get auto-merge to work even though it is enabled on repository level. Require status checks is enabled but cannot add a Test workflow as required as it does not show up in the picker.

chadlwilson commented 4 months ago

@roele Yes, I agree - I am explaining for @jdx how to disable the status check so the workflow can continue to push to main directly for the 12-hourly updates, on the assumption that he's perhaps not familiar and enabled this some time ago and then forgot about it :-)

It's literally a couple of button clicks to disable a status check from being required/un-"peotect" a branch. Note the very important text:

When enabled, commits must first be pushed to another branch [...]

This is why the required status check broke the update automation.

What I'm trying to say is that for any other remaining PRs, the consequence of making the status check no longer "required" is that PRs submitted to fix things manually (perhaps in the scripting or the github actions automation) aren't automatically blocked from clicking the merge button.

Branch protection is pretty limited anyway. A repo admin can still override it and force the merge unless "Do not allow bypassing the above settings" is enabled - all it does is force a "wait, status checks didn't pass/run" stop point, and force a bit more discipline on any outside collaborators.

Just disabling the status check seems infinitely easier than trying to set up a whole PR and auto-merge flow and with very limited downsides in this case. If the status check is simply disabled the automation should flow through normally.

Require status checks is enabled but cannot add a Test workflow as required as it does not show up in the picker.

They only appear in the picker when they have been run once, but you do seem to have done so, so it should be there. Weird. But yes, having spent a lot of time with it, the UI is pretty annoying (any the required checks unmaintainble in practice when using matrix builds that change often).