search

carpentries / workbench

Repository for Discussions and Materials about The Carpentries Workbench
https://carpentries.github.io/workbench/
Creative Commons Attribution 4.0 International
17 stars 7 forks source link

link to migration guide #79

Closed tobyhodges closed 1 month ago

tobyhodges commented 5 months ago

Link to the Migrating lessons from the previous infrastructure workflow page.

ErinBecker commented 1 month ago

Thank you for this documentation update @tobyhodges. Unfortunately, I was so late to reviewing this that the check logs are no longer available and there is a failed check. I think closing and reopening this PR will re-trigger the checks.

ErinBecker commented 1 month ago

Closing and re-opening did indeed retrigger the checks, but they still failed. According to logs, this seems to be a problem with permissions for the github-actions bot.

Screenshot 2024-06-07 at 4 26 29 PM

Which seems to traceback to the Node.js deprecation errors reported elsewhere and tentatively solved by @tobyhodges in this other neglected PR as well as by @Bisaloo in this PR.

GitHub's instructions for updating to Node.js v20 specifies two required parameters - using which this is covered by https://github.com/carpentries/actions/pull/91, and main, which refers back to the action code. @tobyhodges also requests testing in his PR, but unfortunately I have no idea how to test actions. Need to figure that out . . .

It's too late on a Friday afternoon for me to figure this out right now, so I'm adding near the top of @froggleston and my to-work-on-together list, as this issue is as @tobyhodges and @bisaloo have pointed out, impacting multiple workflows and has been going on for 6 months now. 😮‍💨

ErinBecker commented 1 month ago

I came up with a way to test this - it might not be the most sophisticated, but it seems to have worked.

1) I forked both this repo and the actions repo. 2) I merged https://github.com/carpentries/actions/pull/91 into my actions fork. 3) I copied the changes from this PR (#79) to my workbench fork (https://github.com/ErinBecker/workbench/pull/1) and confirmed that checks pass.

Based on these results, I'm going to merge https://github.com/carpentries/actions/pull/91 and then re-run the checks here. I think it should work! 🤞

froggleston commented 1 month ago

I think I've fixed this (https://github.com/carpentries/workbench/commit/767421c3d627de35a299cacea13745d30b1249fb) with explicit permissions needing to be set for the deployment step. Now, I don't know why this has not been required up til now (the automated workflow runs seem to work, but the manually/PR based ones like this seem to fail). However, all seems to work now, so this should be safe to merge I think.

froggleston commented 1 month ago

I simply can't get this to build correctly from the PR, but maybe that's because it is a PR from a fork rather than a push to main. I suggest merging this anyway as the change isn't build or deployment related!