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

feat(ci): refactor build workflow #5572

Merged
merged 58 commits into from
Sep 22, 2022

Conversation

jsjoeio
Copy link
Contributor

@jsjoeio jsjoeio commented Sep 19, 2022

This PR does some major refactoring to our build.yaml workflow, specifically the prebuild step. The two themes of this PR are:

  • parallelization
  • fail-fast

It splits up the prebuild jobs - lint, format, etc - and runs them in separate jobs so they can run in parallel. It also adds a new step to each which cancels the entire workflow if any of them fail. This way, we can fail-fast and not waste developer time or CI minutes. Plus, it allows us to run these prebuild checks in parallel to building code-server which means faster feedback loop.

Fixes N/A

@jsjoeio jsjoeio self-assigned this Sep 19, 2022
@jsjoeio jsjoeio requested a review from a team as a code owner September 19, 2022 19:55
@jsjoeio jsjoeio marked this pull request as draft September 19, 2022 19:58
.github/workflows/build.yaml Outdated Show resolved Hide resolved
.github/workflows/build.yaml Outdated Show resolved Hide resolved
.github/workflows/build.yaml Outdated Show resolved Hide resolved
.github/workflows/build.yaml Outdated Show resolved Hide resolved
.github/workflows/build.yaml Outdated Show resolved Hide resolved
ci/dev/helm.sh Outdated Show resolved Hide resolved
ci/dev/test-integration.sh Outdated Show resolved Hide resolved
test/integration/test-plugin/src/index.ts Outdated Show resolved Hide resolved
test/unit/node/app.test.ts Outdated Show resolved Hide resolved
@jsjoeio jsjoeio marked this pull request as ready for review September 19, 2022 20:12
@codecov
Copy link

codecov bot commented Sep 19, 2022

Codecov Report

Merging #5572 (5f8beae) into main (4223cf6) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #5572   +/-   ##
=======================================
  Coverage   72.44%   72.44%           
=======================================
  Files          30       30           
  Lines        1673     1673           
  Branches      366      366           
=======================================
  Hits         1212     1212           
  Misses        398      398           
  Partials       63       63           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4223cf6...5f8beae. Read the comment docs.

Copy link
Member

@code-asher code-asher left a comment

Choose a reason for hiding this comment

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

Looks good to me, I am going to run it on my branch again to double check the changed files stuff then will approve.

- name: Run integration tests on standalone release
run: yarn test:integration
- name: Run native module tests on standalone release
run: yarn test:native
Copy link
Member

Choose a reason for hiding this comment

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

I like the rename.

@code-asher
Copy link
Member

The installation tests are failing but I think it is due to an older change, not this PR. I will push up a PR after this merges to fix it.

I am getting lint and build errors but I am not sure why...maybe unrelated to this PR? Investigating.

The Typescript lint check seems to run every time now?

@code-asher
Copy link
Member

code-asher commented Sep 22, 2022

False alarm on the lint step, I thought the changed files worked from commit to next commit but I think it compares to main each time. So I think it is working.

I think we might need fetch-depth: 2 though for when the branch gets merged otherwise the checks would not run on main (they would only run on the PR) based on:

IMPORTANT: For push events you need to include fetch-depth: 0 OR fetch-depth: 2 depending on your use case.

@jsjoeio
Copy link
Contributor Author

jsjoeio commented Sep 22, 2022

So I think it is working.

Nice!

I think we might need fetch-depth: 2

Ah interesting. If understanding right, you want me to add that to every step in build.yaml where we checkout code-server?

@code-asher
Copy link
Member

If understanding right, you want me to add that to every step in build.yaml where we checkout code-server?

Only on the steps with the changed-files action (so lint-ts and lint-helm).

@jsjoeio
Copy link
Contributor Author

jsjoeio commented Sep 22, 2022

Only on the steps with the changed-files action (so lint-ts and lint-helm).

Done!

Copy link
Member

@code-asher code-asher left a comment

Choose a reason for hiding this comment

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

My build issues seem to have disappeared so maybe it was a cache issue? If it crops up again we can take another look but I think we are good to go. Nicely done!

@jsjoeio jsjoeio merged commit 51677f0 into coder:main Sep 22, 2022
@jsjoeio jsjoeio deleted the jsjoeio/refactor-workflows branch September 22, 2022 19:33
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.

2 participants