ci(github): migrate ci from circleci to github #1852

rfprod · 2022-06-08T12:08:12Z

reimplement circleci workflows using github actions ci;
clean up github workflows, add comments to justify suggestions;
implement a reusable github workflow that might be used later, add explanatory comments;
use a reusable workflow to setup all jobs;
use a matrix strategy to parallelize scripts execution;
echo explanation of the find ... shred ... command;
cache @ngxs build artifacts, add a reusable workflow to check for build artifacts;
cache app/integrations dists (needed for bundlesize job);
convert reusable workflows to composite actions;

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/ngxs/store/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Implementation of CI workflows using GitHub Actions CI.

[ ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[x] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

CircleCI does not always work as expected.

Issue Number: N/A

What is the new behavior?

CircleCI workflows were reimplementd using GitHub Actions CI.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

There're some TODOs that should be reviewed and addressed in the future.
GitHub Actions CI workflow steps that publish packages to NPM are commented out.
Before uncommenting the GitHub Actions CI workflow steps that publish packages to NPM, the new workflows must be tested first.

markwhitfeld

Great start so far!
I think we should also leave the Circle CI stuff in place too so that we have 2 sets of CI running (because, why not 😉 ).
Some comments:

I see quite a bit of repetition between these files, do github actions have a way of sharing steps, etc between multiple workflows (forgive my ignorance, and inexperience with GH actions if it shows).
Is it possible to do the same as what we have in CircleCI, where the build happens, and then the tasks all happen mainly in parallel where they don't depend on each other?

rfprod · 2022-06-08T20:26:21Z

Great start so far! I think we should also leave the Circle CI stuff in place too so that we have 2 sets of CI running (because, why not wink ). Some comments:

I see quite a bit of repetition between these files, do github actions have a way of sharing steps, etc between multiple workflows (forgive my ignorance, and inexperience with GH actions if it shows).

Is it possible to do the same as what we have in CircleCI, where the build happens, and then the tasks all happen mainly in parallel where they don't depend on each other?

@markwhitfeld there are reusable workflows, but it might be a bit more complex to read and maintain it for someone inexperienced with GH actions. I will try to make it as simple as possible, need to check if its possible to share step execution results between reusable workflows. This feature, reusable workflows, was introduced relatively recently.
Some links:

rfprod · 2022-06-08T21:51:12Z

@markwhitfeld please check my latest commit in this branch.

markwhitfeld

I have left a few comments here, but we can chat about the details on a call 👍

.github/workflows/get-variables.yml

.github/workflows/pr-validation.yml

markwhitfeld · 2022-06-10T10:05:31Z

Here are some great articles that have some awesome recommendations, and security guidances for Github Actions:

arturovt

This looks great! I have a question: could you leave comments to commands like find ~/.npmrc -exec shred -fuz {} +? This would be obvious for reviewers on what this command is doing and will be easier to maintain it in the future, just because everyone will know what it's responsible for.
E.g. this would be great:

# This command is doing ..... `shred -fuz {}` is doing blah blah....
find ~/.npmrc -exec shred -fuz {} +

rfprod · 2022-06-11T10:16:29Z

This looks great! I have a question: could you leave comments to commands like find ~/.npmrc -exec shred -fuz {} +? This would be obvious for reviewers on what this command is doing and will be easier to maintain it in the future, just because everyone will know what it's responsible for. E.g. this would be great:
# This command is doing ..... `shred -fuz {}` is doing blah blah....
find ~/.npmrc -exec shred -fuz {} +

updated

.github/workflows/release.yml

markwhitfeld · 2022-06-11T13:05:21Z

.github/workflows/trunk.yml

+      - name: Development release - publish development builds to all @ngxs packages
+        run: |
+          echo "//registry.npmjs.org/:_authToken=${NPM_TOKEN}" > ~/.npmrc
+          yarn publish:dev
+          echo "Security: remove ~/.npmrc from the runner overwriting it with zeros several times."
+          find ~/.npmrc -exec shred -fuz {} +


Same here, we need to copy in the build artefacts before this step

same as above - 016b129

markwhitfeld

Thank you so much for this!
Just a few more tweaks...
I'm still battling to get GitHub to allow this to run. Maybe you can experiment and test this in your own clean repo with some spoofed data and scripts. Then you are not blocked with not being able to run it in the meantime. What do you think?

markwhitfeld · 2022-06-11T17:44:30Z

.github/workflows/_setup.yml

+            ~/.cache
+            ./node_modules
+            ./@ngxs
+          key: ${{ runner.os }}-node-${{ matrix.node-version }}-yarn-${{ hashFiles('**/yarn.lock') }}-branch-${{ steps.get-variables.outputs.shortref }}-workspace-${{ steps.get-variables.outputs.commitsha }}


We need the NODE_VERSION variable here and below too, still referencing the matrix strategy.

@markwhitfeld fixed

markwhitfeld · 2022-06-11T17:47:33Z

.github/workflows/pr-validation.yml

+          chmod +x /tmp/cc-test-reporter
+          /tmp/cc-test-reporter after-build --coverage-input-type lcov --exit-code 0
+        env:
+          CC_TEST_REPORTER_ID: 3f4c9a9d57ded045e0f9ab5d23e5bbcbf709bb85637bea555f1233e72134b818 # TODO: better store it in the repository secrets. Name the variable: CC_TEST_REPORTER_ID. Then delete this line and uncomment the next one


Yeah, can we move this to secrets

markwhitfeld · 2022-06-11T17:52:57Z

.github/workflows/_setup.yml

+          path: |
+            ~/.cache
+            ./node_modules
+            ./@ngxs


Just wondering if putting this in the cache is potentially a security concern.
Trying to think if this can be exploited in any way by someone writing malicious code to the cache between our build and deploy.
The next alternative I think about is using the store and repetitive artefact action, but is that safer?
Possibly we should actually run the build again in the publish job to be safest.
@arturovt & @rfprod what do you think?

@markwhitfeld I think that in this particular case, the likelihood that something will go wrong with the artifacts is extremely small.
Here's why:

when the workflow (trunk or release) is kicked off, the first job is *-build (trunk-build or release-build) <- *-build jobs refresh workspace-cache by rebuilding all packages and saving it to the cache.

Then several more jobs are executed: *-test, *-integration-test, *-bundlesize.

Then the *-publish job is executed. The publish job uses build artifacts that were generated by the *-build job (see point 1 of this list).

A possible attack surface is in between of build and publish, it's point 2 in the list above.
The attacker should be very persistent and determined to mess up the publish step.
If we assume that the workspace artifacts (including the build artifacts) are corrupted before *-build, they will be fixed after the *-build job completion.

I think, with the same probability someone could tap into the runner and corrupt the build artifacts. Of course there is a probability of this, but it tends to zero.

If you want an additional layer of protection to be sure that the build artifacts were not changed after caching and before publishing, we can hash the content of each directory with the build artifacts and save hashing results as part of the workspace-cache. The _check-build-artifacts.yml workflow can be configured to verify hashes in addition to ensuring that the directories exist.

PS: In addition to hashing, we can encrypt the build artifacts before saving it to cache. This will increase complexity, but make it much harder for build artifacts to become corrupted, even if they are stored externally. The procedure is not be very complex, and it can be fully automated. The process is like:

before save: archive artifacts, e.g. tar.gz -> encrypt (gpg, passphrase) -> hash encrypted archives

before use: verify hashes of the encrypted archives -> decrypt (gpg, passphrase) -> unarchive artifacts

The gpg passphrase can be stored in the repository secrets.

Thank you for your very well-thought-out response.
I think that it is good to bear this in mind, but I don't think it is essential right now.
I think that the simplest approach is to keep what we have here, using the cache to make the built files available to the next job, and then in the publish step, we could just build it again. It is repeating the work, but everything preceding the publish leveraged the built files as part of the validation leading up to the publish. It is OK if the publish repeats a number of steps to be safer because it is on the tail end of the workflow and will not slow the PR workflow down.

.github/workflows/pr-validation.yml

.github/workflows/trunk.yml

markwhitfeld · 2022-06-11T18:28:02Z

@rfprod after reading this article: https://github.blog/2021-11-29-github-actions-reusable-workflows-is-generally-available/
I am wondering if I am mistaken about reusable workflows, and that they are actually run in their own runner.
Potentially composite actions are what we need.
I think playing with this in a test repo is really critical to figuring out the best combination. We won't know until we try it.

rfprod · 2022-06-11T18:41:17Z

@rfprod after reading this article: https://github.blog/2021-11-29-github-actions-reusable-workflows-is-generally-available/ I am wondering if I am mistaken about reusable workflows, and that they are actually run in their own runner. Potentially composite actions are what we need. I think playing with this in a test repo is really critical to figuring out the best combination. We won't know until we try it.

@markwhitfeld As I mentioned during our call, I haven't used reusable workflows like this before. :) I'll try if this approach actually works, maybe tomorrow.
As far as I know, each reusable workflow has its own isolated context in every sense, but maybe something has changed or I did not fully understand everything. The only way I am aware of to share artifacts between reusable workflows and not reusable workflows is upload-artifact + download-artifact. actions/cache is something that's generally used in workflows that are not reusable.

PS: check my latest commits

markwhitfeld

Looking great.
Now just to see it running!!?!
Hopefully this approved review opens that option...
PS. Team, please wait for me to merge once we have potentially iterated a few times with seeing it in action.

.github/workflows/release.yml

.github/workflows/pr-validation.yml

- [x] reimplement circleci workflows using github actions ci;

- [x] add a cleanup command that removes .npmrc after publishing packages;

- [x] clean up workflows, add comments to justify suggestions; - [x] implement a reusable workflow that might be used later, add explanatory comments;

Co-authored-by: Mark Whitfeld <[email protected]>

- [x] use a reusable workflow to setup all jobs; - [x] use a matrix strategy to parallelize scripts execution;

- [x] add a reusable workflow to check for build artifacts;

rfprod · 2022-06-12T21:07:49Z

Looking great. Now just to see it running!!?! Hopefully this approved review opens that option... PS. Team, please wait for me to merge once we have potentially iterated a few times with seeing it in action.

@markwhitfeld pr-validation can be tested in my fork, see here rfprod#1.
See all test runs starting form the first one before I pushed 2dd5a47 here -> https://github.com/rfprod/store/actions
Seems like it won't work in this repo before this PR is merged.
I am reviewing and addressing bugs in workflows to make it work.

- [x] all actions must required build so that cache is correct;

rfprod · 2022-06-12T23:26:31Z

@markwhitfeld cache is immutable. Check my latest changes. build should go before all other jobs. Bundlesize wont work for integration tests unless upload/download artifact action is used in addition to workspace cache to pass additional artifacts to bundlesize. Jest cache won't persist between workflow runs as part of the workspace cache unless test:ci is executed in the same workflow as build.

https://github.com/rfprod/store/actions/runs/2485166750

markwhitfeld · 2022-06-13T12:21:34Z

It is looking great. So nice to see it running!
It's fine that the jest cache doesn't persist. Unless I am missing where that would be an advantage?
I think that we should use the upload/download artifact action to carry the build outputs of the integration tests (and potentially the main build too) across the workflow.
I think we would probably use the wildcard approach described here (that maintains folder structure):
https://github.com/actions/upload-artifact#upload-using-multiple-paths-and-exclusions
It looks like artefacts are mutable. So we can progressively update this from the jobs that are run.
Potentially the _setup.yml could always restore these artifacts (with the target as the root).

rfprod · 2022-06-13T13:49:02Z

It's fine that the jest cache doesn't persist. Unless I am missing where that would be an advantage?

@markwhitfeld faster subsequent pipeline runs are the advantage. For example, if you modify source code of only one ngxs package, jest will process other libraries faster if there is cache of the previous test runs.

It looks like artefacts are mutable. So we can progressively update this from the jobs that are run.

I'll add artifacts to the workflows later today. Will post an update here after.

- [x] add two composite actions to upload/download integration test artifacts;

rfprod · 2022-06-13T23:16:47Z

@markwhitfeld all jobs green.

Pipeline run https://github.com/rfprod/store/actions/runs/2491727586
Integration test artifacts b3ba642

markwhitfeld · 2022-06-16T01:18:25Z

.github/workflows/pr-validation.yml

+        with:
+          fetch-depth: 0


This seems like a good default. Is there a reason why this is not done in the other workflows for the checkout step?
If they can all be the same, then this could actually be moved into the setup composite action.

@markwhitfeld fetch-depth: 0 stands for all history for all branches and tags. It should be a bit slower and it pulls much more data than needed for other workflows than those that validate pull requests. From my experience with common repos and monorepos, it is generally not needed to pull all git history in the trunk or release workflows, because all tests that might require full commit history of all branches and tags (e.g. validate commit messages using commitlint) were done earlier during the corresponding PR validation.

If you think the default fetch-depth: 0 makes sense, so be it.

Ok. Makes sense. Thanks!

@markwhitfeld one more thing about the actions/checkout action.

Sources should be checked out prior to any steps that use composite actions like ./github/actions/..., because relative references requires already checkout out sources.
If you want to wrap the step that check out sources as a separate composite action, the only way to reference this composite action that will work, if I understand correctly, is ngxs/store/.github/actions/...

Oh, hahaha. Yes, that makes total sense!

* ci(github): migrate ci from circleci to github - [x] reimplement circleci workflows using github actions ci; * ci(cleanup): remove .npmrc after publishing packages - [x] add a cleanup command that removes .npmrc after publishing packages; * ci(github): revise github actions ci workflows - [x] clean up workflows, add comments to justify suggestions; - [x] implement a reusable workflow that might be used later, add explanatory comments; * Update .github/workflows/get-variables.yml Co-authored-by: Mark Whitfeld <[email protected]> * Update .github/workflows/pr-validation.yml Co-authored-by: Mark Whitfeld <[email protected]> * ci(premerge): upload premerge unit test coverage results to code climate * ci(dry): setup jobs with a reusable workflow, use matrix for scripts - [x] use a reusable workflow to setup all jobs; - [x] use a matrix strategy to parallelize scripts execution; * ci(logging): echo explanation of the 'find ... shred ...' command * ci(artifacts): check build artifacts workflow - [x] add a reusable workflow to check for build artifacts; * ci(setup): fix setup workflow - get node version from inputs * ci(artifacts): save app/integrations dists to workspace cache * ci(actions): convert reusable workflows to composite actions * ci(actions): fix composite actions * ci(actions): update comments in action configs * ci(docs): fix comments in composite actions * ci(syntax): fix yaml syntax errors * ci(checkout): checkout sources before calling the setup action * ci(inputs): fix the setup action inputs * ci(syntax): fix the setup action syntax * ci(test): test:types needs *-build, move it to integration-test * ci(cache): fix workspace cache paths * ci(cache): exclude node_modules in the integrations dir from cache * ci(cache): fix workspace cache, remove unneeded code * ci(cache): fix workspace cache - [x] all actions must required build so that cache is correct; * ci(actions): add composite actions to upload/download artifacts - [x] add two composite actions to upload/download integration test artifacts; * fix: bundlesize step order (release & trunk) Co-authored-by: Mark Whitfeld <[email protected]>

markwhitfeld reviewed Jun 8, 2022

View reviewed changes

rfprod force-pushed the github-ci branch from 838c621 to ce163dc Compare June 9, 2022 12:10

markwhitfeld requested changes Jun 10, 2022

View reviewed changes

rfprod marked this pull request as ready for review June 10, 2022 15:36

rfprod requested review from arturovt, kuncevic, Carniatto, poloagustin, joaqcid and splincode as code owners June 10, 2022 15:37

arturovt reviewed Jun 10, 2022

View reviewed changes

markwhitfeld requested changes Jun 11, 2022

View reviewed changes

arturovt linked an issue Jun 12, 2022 that may be closed by this pull request

🚀[FEATURE]: Move to Github Actions #1705

Closed

markwhitfeld approved these changes Jun 12, 2022

View reviewed changes

.github/workflows/release.yml Show resolved Hide resolved

.github/workflows/pr-validation.yml Show resolved Hide resolved

rfprod and others added 9 commits June 12, 2022 23:48

ci(github): migrate ci from circleci to github

a334315

- [x] reimplement circleci workflows using github actions ci;

ci(cleanup): remove .npmrc after publishing packages

9cee7e1

- [x] add a cleanup command that removes .npmrc after publishing packages;

ci(github): revise github actions ci workflows

5858aeb

- [x] clean up workflows, add comments to justify suggestions; - [x] implement a reusable workflow that might be used later, add explanatory comments;

Update .github/workflows/get-variables.yml

4b50606

Co-authored-by: Mark Whitfeld <[email protected]>

Update .github/workflows/pr-validation.yml

5fb7e8d

Co-authored-by: Mark Whitfeld <[email protected]>

ci(premerge): upload premerge unit test coverage results to code climate

5058adf

ci(dry): setup jobs with a reusable workflow, use matrix for scripts

4ffe17f

- [x] use a reusable workflow to setup all jobs; - [x] use a matrix strategy to parallelize scripts execution;

ci(logging): echo explanation of the 'find ... shred ...' command

0a08b13

ci(artifacts): check build artifacts workflow

edff7ff

- [x] add a reusable workflow to check for build artifacts;

rfprod added 6 commits June 12, 2022 23:48

ci(setup): fix setup workflow - get node version from inputs

9130bb8

ci(artifacts): save app/integrations dists to workspace cache

f63ad6c

ci(actions): convert reusable workflows to composite actions

1d6db8b

ci(actions): fix composite actions

2de8269

ci(actions): update comments in action configs

4f6c585

ci(docs): fix comments in composite actions

600c297

rfprod force-pushed the github-ci branch from 651ae3e to 600c297 Compare June 12, 2022 20:48

ci(syntax): fix yaml syntax errors

2dd5a47

rfprod mentioned this pull request Jun 12, 2022

GitHub CI test run (pr-validation) rfprod/store#1

Closed

ci(checkout): checkout sources before calling the setup action

b8c3086

rfprod added 7 commits June 13, 2022 00:20

ci(inputs): fix the setup action inputs

d242816

ci(syntax): fix the setup action syntax

c0d9f05

ci(test): test:types needs *-build, move it to integration-test

7603b47

ci(cache): fix workspace cache paths

4bee88b

ci(cache): exclude node_modules in the integrations dir from cache

95c3034

ci(cache): fix workspace cache, remove unneeded code

8b53500

ci(cache): fix workspace cache

a75f815

- [x] all actions must required build so that cache is correct;

ci(actions): add composite actions to upload/download artifacts

b3ba642

- [x] add two composite actions to upload/download integration test artifacts;

rfprod force-pushed the github-ci branch from d45ca83 to b3ba642 Compare June 13, 2022 23:07

fix: bundlesize step order (release & trunk)

5eb6e7c

markwhitfeld reviewed Jun 16, 2022

View reviewed changes

markwhitfeld approved these changes Jun 16, 2022

View reviewed changes

markwhitfeld merged commit 327646e into ngxs:master Jun 16, 2022

rfprod deleted the github-ci branch June 18, 2022 17:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ci(github): migrate ci from circleci to github #1852

ci(github): migrate ci from circleci to github #1852

rfprod commented Jun 8, 2022 •

edited

Loading

markwhitfeld left a comment

rfprod commented Jun 8, 2022 •

edited

Loading

rfprod commented Jun 8, 2022 •

edited

Loading

markwhitfeld left a comment

markwhitfeld commented Jun 10, 2022

arturovt left a comment

rfprod commented Jun 11, 2022

markwhitfeld Jun 11, 2022

rfprod Jun 11, 2022

markwhitfeld left a comment

markwhitfeld Jun 11, 2022

rfprod Jun 11, 2022

markwhitfeld Jun 11, 2022

markwhitfeld Jun 11, 2022

rfprod Jun 11, 2022 •

edited

Loading

markwhitfeld Jun 12, 2022 •

edited

Loading

markwhitfeld commented Jun 11, 2022

rfprod commented Jun 11, 2022 •

edited

Loading

markwhitfeld left a comment

rfprod commented Jun 12, 2022 •

edited

Loading

rfprod commented Jun 12, 2022

markwhitfeld commented Jun 13, 2022

rfprod commented Jun 13, 2022 •

edited

Loading

rfprod commented Jun 13, 2022

markwhitfeld Jun 16, 2022 •

edited

Loading

rfprod Jun 16, 2022 •

edited

Loading

markwhitfeld Jun 16, 2022

rfprod Jun 16, 2022

markwhitfeld Jun 16, 2022

ci(github): migrate ci from circleci to github #1852

ci(github): migrate ci from circleci to github #1852

Conversation

rfprod commented Jun 8, 2022 • edited Loading

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

markwhitfeld left a comment

Choose a reason for hiding this comment

rfprod commented Jun 8, 2022 • edited Loading

rfprod commented Jun 8, 2022 • edited Loading

markwhitfeld left a comment

Choose a reason for hiding this comment

markwhitfeld commented Jun 10, 2022

arturovt left a comment

Choose a reason for hiding this comment

rfprod commented Jun 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markwhitfeld left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfprod Jun 11, 2022 • edited Loading

Choose a reason for hiding this comment

markwhitfeld Jun 12, 2022 • edited Loading

Choose a reason for hiding this comment

markwhitfeld commented Jun 11, 2022

rfprod commented Jun 11, 2022 • edited Loading

markwhitfeld left a comment

Choose a reason for hiding this comment

rfprod commented Jun 12, 2022 • edited Loading

rfprod commented Jun 12, 2022

markwhitfeld commented Jun 13, 2022

rfprod commented Jun 13, 2022 • edited Loading

rfprod commented Jun 13, 2022

markwhitfeld Jun 16, 2022 • edited Loading

Choose a reason for hiding this comment

rfprod Jun 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfprod commented Jun 8, 2022 •

edited

Loading

rfprod commented Jun 8, 2022 •

edited

Loading

rfprod commented Jun 8, 2022 •

edited

Loading

rfprod Jun 11, 2022 •

edited

Loading

markwhitfeld Jun 12, 2022 •

edited

Loading

rfprod commented Jun 11, 2022 •

edited

Loading

rfprod commented Jun 12, 2022 •

edited

Loading

rfprod commented Jun 13, 2022 •

edited

Loading

markwhitfeld Jun 16, 2022 •

edited

Loading

rfprod Jun 16, 2022 •

edited

Loading