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

Update nodejs actions to node16 #548

Merged

Conversation

jbpaux
Copy link
Contributor

@jbpaux jbpaux commented Jan 4, 2023

What this PR does / why we need it:
This PR updates nodejs actions used by the different workflows to node16 as node12 is deprecated.
It updates both embedded actions in the projects and dependent actions (except actions-rs actions, see below).
It closes #537

Special notes for your reviewer:
actions-rs actions (audit-check and toolchain) don't have node16 compatible versions available and the maintainer is gone. I would advise to switch to other actions. I found one for the toolchain: the dtolnay/rust-toolchain one but none for the audit-check. We can maybe tackle that in another PR.

If applicable:

  • this PR has an associated PR with documentation in akri-docs
  • this PR contains unit tests
  • added code adheres to standard Rust formatting (cargo fmt)
  • code builds properly (cargo build)
  • code is free of common mistakes (cargo clippy)
  • all Akri tests succeed (cargo test)
  • inline documentation builds (cargo doc)
  • all commits pass the DCO bot check by being signed off -- see the failing DCO check for instructions on how to retroactively sign commits

@jbpaux
Copy link
Contributor Author

jbpaux commented Jan 4, 2023

Not sure why the "add-label-missing-comment" job is failing. Apparently the GITHUB_TOKEN doesn't have enough permissions or we are talking about my permissions maybe.

@kate-goldenring
Copy link
Contributor

kate-goldenring commented Jan 4, 2023

@jbpaux that is weird that it isnt commenting to the PR as expected. I tried to rerun it and it doesn't work either

@kate-goldenring
Copy link
Contributor

/add-build-dependency-containers-label

@github-actions
Copy link
Contributor

github-actions bot commented Jan 4, 2023

👋 Added [build dependency containers] label :)!

1 similar comment
@github-actions
Copy link
Contributor

github-actions bot commented Jan 4, 2023

👋 Added [build dependency containers] label :)!

@kate-goldenring
Copy link
Contributor

@vincepnguyen do you have a sense of why the build dependency containers action is not working (#426)

@jbpaux
Copy link
Contributor Author

jbpaux commented Jan 4, 2023

When I see the GITHUB_TOKEN permissions, I see it only has read access to issues and the error is a 403 on a POST request to add comment https://github.com/project-akri/akri/actions/runs/3837765961/jobs/6538055513 while when you launched it it had write permissions https://github.com/project-akri/akri/actions/runs/3839766025/jobs/6537971838

@jbpaux
Copy link
Contributor Author

jbpaux commented Jan 25, 2023

any idea on how to get this merged ? 😁

@kate-goldenring
Copy link
Contributor

@adithyaj do you know if anyone can put eyes on this to get this merged soon?

@yujinkim-msft
Copy link
Contributor

@jbpaux any chance you can kick off the checks for this PR again? we would love to have it merged for this next release!

@jbpaux
Copy link
Contributor Author

jbpaux commented Apr 11, 2023

I'd love to but I can't, only maintainers can

@yujinkim-msft
Copy link
Contributor

@jbpaux are you able to kick off the checks in your local fork of your code?

@kate-goldenring
Copy link
Contributor

I am not sure why i haven't been able to manually kick them off but we needed to add the same version flag anyways and that kicked off some

@yujinkim-msft
Copy link
Contributor

@jbpaux looks like the build OpenCV check is failing? can you check to see if you can resolve this? then we can approve and merge

@kate-goldenring
Copy link
Contributor

We haven't rebuilt the openCV container in a couple months. The container is failing to build and we likely need to update the Dockerfile which is a two part process, as changes to our intermediate containers must be made in a separate pr: https://github.com/project-akri/akri/blob/main/build/containers/intermediate/Dockerfile.opencvsharp-build#L8. Furthermore, this container takes notoriously long to build so the dev cycle is long on this one. @jbpaux you are welcome to take this on if interested; however, i'd recommend that we let the building of these intermediate containers fail and still merge it. Then we will continue to use the last built image (2 months ago) and can put up an issue to fix this separately.

@jbpaux
Copy link
Contributor Author

jbpaux commented Apr 12, 2023

Yes I think it's better to handle that in another PR as it's not related to the CI actions.
I also had a note in the review about the actions-rs actions that don't have node16 equivalent but like I said, maybe tackle that in another PR too.

@jbpaux jbpaux force-pushed the features/upgrade-nodejs-actions branch from 24feffb to e51a6e7 Compare April 12, 2023 12:19
@jbpaux
Copy link
Contributor Author

jbpaux commented Apr 12, 2023

I've rebased to the latest changes, should be good to merge then

@kate-goldenring
Copy link
Contributor

@jbpaux good call out. Created an issue to track it here #579

@kate-goldenring
Copy link
Contributor

created an issue to track fixing the opencv build #580

@yujinkim-msft yujinkim-msft merged commit 2497037 into project-akri:main Apr 12, 2023
@jbpaux jbpaux deleted the features/upgrade-nodejs-actions branch April 12, 2023 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update node build flow dependency from node12 to node16
3 participants