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

Build: add GitHub Actions workflow to update Filestash #5434

Merged
merged 1 commit into from
Mar 8, 2024

Conversation

timmywil
Copy link
Member

@timmywil timmywil commented Mar 8, 2024

Summary

Replaces the jenkins workflow to update git versions of jQuery on the Filestash server. The filestash environment is already set up in the jQuery repo settings and includes the necessary SSH private key and filestash server URL.

Here's a sample of the workflow running as a dry run:
https://github.com/timmywil/jquery/actions/runs/8205613186/job/22442850764

We should disable the jenkins workflow before merging.

Checklist

@timmywil timmywil requested review from Krinkle and mgol March 8, 2024 15:49
@timmywil timmywil added the Build label Mar 8, 2024
@timmywil timmywil added this to the 4.0.0 milestone Mar 8, 2024
@Krinkle
Copy link
Member

Krinkle commented Mar 8, 2024

The following look great:

  • Expose private key as "secret" rather than as "environment var", that way we control when it becomes visible, thus not making it visible during earlier steps.
  • Install ssh key not until after install/build steps.
  • Further limit secrets to a declared group (confusingly called "enviornment"), and the job to a given branch.

The following could tighten it further:

  • Configure the secret in the repo setting by branch (i.e. protected-only, or list "main" there). This because as-is the restriction is in the file itself, which feels insufficient. I.e. if it is modified to allow branch X and pushed to X, or if another job is modified to enable the same "filestash" group secrets, that would bypass it. Putting the restriction also/instead in the repo settings would harden this.
  • Consider removing use of actions/setup-node when the purpose of a build isn't to test specific or multiple Node.js versions. This action is pretty convoluted and heavy. The ubuntu image already comes with an LTS version of nodejs pre-installed. For Ubuntu 22 (ubuntu-22, ubuntu-latest) that is currently Node.js 18. https://github.com/actions/runner-images/blob/ubuntu22/20240218.1/images/ubuntu/Ubuntu2204-Readme.md. I imagine Ubuntu 24 will come soon with Node.js 20 (assuming it follows https://packages.debian.org/experimental/nodejs).

Neither a big deals. LGTM as-is.

@timmywil
Copy link
Member Author

timmywil commented Mar 8, 2024

The first suggestion sounds good to me, but I prefer to have control and consistency over the Node version everywhere we build. I've learned not to trust whatever Node version comes preinstalled in the runner image. We've been bitten by that before, and I don't want to be at the mercy of the image's update schedule. FWIW, that step usually takes less than a second and is the same for all workflows that build.

@timmywil
Copy link
Member Author

timmywil commented Mar 8, 2024

I've applied "protected branches only" to the environment, which includes main and *-stable

Copy link
Member

@mgol mgol left a comment

Choose a reason for hiding this comment

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

LGTM. Let me know when you're ready to merge so that I disable the Jenkins job.

One thing - with the Jenkins job being disabled before merging this, we no longer need to build on Node.js 10. You can remove it here if you want. That said, dropping Node 10 allows us to introduce a few improvements like upgrade Rollup (which I need for the exports improvements PR), switch to node:-prefixed Node.js core modules, using node:fs/promises instead of fs.promises from node:fs, dropping the jenkins npm script and maybe something else. If you'd like, you can group it all in a followup PR. I'd just prefer not to wait too long with dropping the build on Node 10.

@timmywil
Copy link
Member Author

timmywil commented Mar 8, 2024

Thanks for making a list. I'll make those changes in a followup PR.

@timmywil timmywil merged commit 0293d3e into jquery:main Mar 8, 2024
14 checks passed
@timmywil timmywil deleted the filestash branch March 8, 2024 20:07
@timmywil
Copy link
Member Author

timmywil commented Mar 8, 2024

Checked all files and they all updated as expected.

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

Successfully merging this pull request may close these issues.

None yet

3 participants