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

ci: improve github actions #1573

Merged
merged 4 commits into from
Aug 11, 2024

Conversation

dhensby
Copy link
Contributor

@dhensby dhensby commented Sep 14, 2023

  1. The setup-node action automatically stores caches for NPM and uses the same logic as the current action implementation, so there's no need to duplicate it in the action definition.
  2. Remove redundant NPM upgrade step - v6.x of Node is no longer tested/supported.
  3. Linting step does not need complete repository history.
  4. Use setup-sqlserver action to install SQL Server

@codecov
Copy link

codecov bot commented Sep 14, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.25%. Comparing base (af21197) to head (9466826).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1573   +/-   ##
=======================================
  Coverage   78.25%   78.25%           
=======================================
  Files          93       93           
  Lines        4879     4879           
  Branches      937      937           
=======================================
  Hits         3818     3818           
  Misses        762      762           
  Partials      299      299           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dhensby dhensby force-pushed the pulls/action-improvements branch 3 times, most recently from 04e5199 to a4a44cb Compare September 14, 2023 22:13
@arthurschreiber
Copy link
Collaborator

@dhensby Would you mind resolving the conflicts with the latest changes that happened on master?

dhensby added 3 commits August 8, 2024 22:16
The setup-node action automatically stores caches for NPM and uses the
same logic as the current action implementation, so there's no need to
duplicate it in the action definition.
The npm upgrade steps in CI only run if the matrix version for node is 6.x
but 6.x is not a tested version of node in any of the matrix definitions.
The linting step does not need the entire repository history to
run. Only checkout the minimal history to allow faster lint step.
@dhensby dhensby force-pushed the pulls/action-improvements branch from a4a44cb to d3fa7f3 Compare August 8, 2024 21:19
@dhensby
Copy link
Contributor Author

dhensby commented Aug 8, 2024

@arthurschreiber done.

I had also wanted to replace lots of the repeated SQL Server installer code with the setup-sqlserver action, but I'm not sure it's an entirely 1-to-1 replacement

@arthurschreiber
Copy link
Collaborator

@dhensby It'd be great if setup-sqlserver would also fetch the latest updates - otherwise sql server 2022 is broken (TDS protocol version 8 does not work correctly).

@dhensby
Copy link
Contributor Author

dhensby commented Aug 8, 2024

@arthurschreiber - working on it: tediousjs/setup-sqlserver#45

@arthurschreiber
Copy link
Collaborator

I never quite understood why this didn't work out of the box. The installer has an option (UpdateEnabled) which defaults to true, and the default update source is MU (= Microsoft Update). 🤷

@dhensby
Copy link
Contributor Author

dhensby commented Aug 8, 2024

I stopped trying to understand the SQL installer a long time ago! 😬

@dhensby dhensby force-pushed the pulls/action-improvements branch 3 times, most recently from 9714b6b to b59813c Compare August 9, 2024 00:44
@dhensby dhensby force-pushed the pulls/action-improvements branch from b59813c to 9466826 Compare August 9, 2024 00:46
@dhensby dhensby marked this pull request as ready for review August 9, 2024 00:50
@dhensby
Copy link
Contributor Author

dhensby commented Aug 9, 2024

@arthurschreiber looks like it's working, one of the tests hit a timeout and needs restarting

@dhensby
Copy link
Contributor Author

dhensby commented Aug 9, 2024

🎉 we are green

@arthurschreiber arthurschreiber merged commit 03777a7 into tediousjs:master Aug 11, 2024
27 checks passed
Copy link

🎉 This PR is included in version 18.4.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@dhensby dhensby deleted the pulls/action-improvements branch August 11, 2024 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants