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

Modernize CI #1616

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open

Modernize CI #1616

wants to merge 19 commits into from

Conversation

wusatosi
Copy link

@wusatosi wusatosi commented May 2, 2023

This PR updates the auto-build CI pipeline.
Specifically:

  1. Updates actions/checkout
  2. Updates actions/upload-artifact, actions/download-artifact and resolves the breaking change.
  3. Replaces discontinued actions/create-release and actions/upload-release-asset with the GitHub CLI

Successful test on latest commit:
https://github.com/wusatosi/clangd/actions/runs/4874186461

Generated release:
https://github.com/wusatosi/clangd/releases/tag/ci-test-12

Edit: Please review @kadircet @usx95 @sam-mccall

@wusatosi
Copy link
Author

wusatosi commented May 2, 2023

Question for maintainer:

# Because the build takes more than an hour, our GITHUB_TOKEN credentials may
# expire. A token `secrets.RELEASE_TOKEN` must exist with public_repo scope.

Unless one job takes over 24 hours to complete, this is not true, "Before each job begins, GitHub fetches an installation access token for the job. The GITHUB_TOKEN expires when a job finishes or after a maximum of 24 hours."

Should I replace the RELEASE_TOKEN with GITHUB_TOKEN of appropriate permissions?

Note: if this change go through we also need to delete:

# Uploading releases needs a per-job token that expires after an hour.

@wusatosi
Copy link
Author

wusatosi commented May 3, 2023

Testing on: https://github.com/wusatosi/clangd/actions/runs/4874186461

Edit: Test passed, ready for review

@wusatosi wusatosi marked this pull request as ready for review May 3, 2023 20:09
Copy link

@aaronmondal aaronmondal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wusatosi I've messaged you on Discord.

@aaronmondal
Copy link

According to OpenSSF security best practices it's generally a good idea to use explicit commit hashes to ensure that the actions themselves are reproducible and robust against malicious activity in the upstream action repos.

See https://github.com/ossf/scorecard/blob/main/docs/checks.md#pinned-dependencies for reasoning. However, I believe this is under the assumption that one has bots tracking these hashes and triggering automatic hash updates. In any case I think it's worth considering.

The OpenSSF also recommends globally setting explicit read-all permissions for every action globally that are only elevated locally for specific steps: https://github.com/ossf/scorecard/blob/main/docs/checks.md#token-permissions

I've just encountered this myself, here is how a pinned version could look like: eomii/rules_ll@c927b80 (that repo is crawled by dependabot and renovate though).

Here is how explicit permissions could look like: eomii/rules_ll@4734461

My comments are not specific to this particular PR, and more so with the workflows in clangd in general, so feel free to ignore it for the sake of this PR.

@aaronmondal
Copy link

@kadircet Would you mind taking a look?

@kadircet
Copy link
Member

sorry for shying away from this review for a while now. any chance you can split those into different changes, updates of actions/checkout, actions/upload-artifact, actions/download-artifact look safe and trivial.

i am not so sure about the change to wusatosi/setup-gh@v1. Why not use https://github.com/softprops/action-gh-release but come up with a new action?

re @aaronmondal, sure using actions from explicit hashes instead of tags makes sense. feel free to send a follow-up patch if @wusatosi don't do that already.

@aaronmondal
Copy link

@kadircet Thanks for taking a look!

I agree that softprops/action-gh-release seems like a better choice since it seems to be the most widely used release action.

@wusatosi
Copy link
Author

wusatosi commented Dec 5, 2023

sorry for shying away from this review for a while now. any chance you can split those into different changes, updates of actions/checkout, actions/upload-artifact, actions/download-artifact look safe and trivial.

i am not so sure about the change to wusatosi/setup-gh@v1. Why not use https://github.com/softprops/action-gh-release but come up with a new action?

re @aaronmondal, sure using actions from explicit hashes instead of tags makes sense. feel free to send a follow-up patch if @wusatosi don't do that already.

Hi, thanks for looking over this pr, I am in the middle of final exams and will update this pr once I have time.

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.

None yet

3 participants