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

SCRUM-3718: upgrade github actions to nodejs 20 #1434

Merged
merged 1 commit into from
Feb 20, 2024
Merged

SCRUM-3718: upgrade github actions to nodejs 20 #1434

merged 1 commit into from
Feb 20, 2024

Conversation

abecerra
Copy link
Contributor

No description provided.

@abecerra abecerra merged commit 1c0f4c8 into alpha Feb 20, 2024
5 checks passed
@abecerra abecerra deleted the SCRUM-3718 branch February 20, 2024 13:39
@oblodgett
Copy link
Member

@mluypaert please let the team approve these type of tickets as this ticket was created only 2 hours ago. And now the UI build fails. We need to discuss this kind of thing as a team before the work gets done. As this now is holding up other work that is more important then a github warning.

@mluypaert
Copy link
Member

@oblodgett Sure, I can hold off on approving future tickets in the curation repo if the A-team feels that's not beneficial. That being said, three thoughts:

  1. I am not responsible for decisions on whether or not and when to merge a ticket, that's up to the PR author (whereas I only served as reviewer and did not hit the merge button after approving)
  2. If the UI build now fails because of this ticket then the PR validation should be updated to include an extra check, as PR validation succeeded (which means there should be no technical reasons not to merge it)
  3. If this PR caused the current UI build failures then that should be easy to fix, just revert the PR merge commit and create a new PR to merge that revert (into alpha).

@oblodgett
Copy link
Member

I agree with your sentiments. However this ticket was created, code pushed, approved, and deployed. Before anyone else on the A-team knew about it. This change requires everyone developing the UI to upgrade to node 20 locally. So if we want to talk about transparency this was not a transparent ticket.

@mluypaert
Copy link
Member

mluypaert commented Feb 20, 2024

I agree with your sentiments. However this ticket was created, code pushed, approved, and deployed. Before anyone else on the A-team knew about it. This change requires everyone developing the UI to upgrade to node 20 locally. So if we want to talk about transparency this was not a transparent ticket.

@oblodgett I think you're misinterpreting the title here. The only changes that were made in this PR were to update third-party workflows we use to ensure those use node 20 rather than node 16 (behind the scenes). The gh-actions workflow code that builds the UI (such as here) has not been changed and still uses node 18. So no UI errors should arise from this PR.

@mluypaert
Copy link
Member

@oblodgett I believe the errors you were referring to were in validating PR #1435? Seems fixed now by changes made to the PR branch there, so not caused by anything here. I agree that I shouldn't approve PRs that would have knock-on effects for the team, but my judgement call here is that there was no risk in approving this PR (as I did review the content and not the title) and it introduced no changes that required team approval, so I can keep approving PRs like this. Let me know if you still feel otherwise.

@oblodgett
Copy link
Member

So... the name of this PR is "upgrade actions to nodejs 20" however I don't see nodejs upgraded here at all. I see the upgrades to the other modules... which I agree should be fine. Maybe this is a lot todo about nothing.

@mluypaert
Copy link
Member

A more appropriate title probably would have been something along the lines of Upgrade third-party gh-actions workflow versions used to remove nodejs 16 dependencies

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants