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 clean up to fix lint task and remove Windows / MacOS from matrix #3122

Merged
merged 4 commits into from Mar 5, 2024

Conversation

sehrope
Copy link
Contributor

@sehrope sehrope commented Jan 5, 2024

Couple of commits here to clean up some CI issues.

  • First removes the old Travis-CI config that hasn't been used in ages.
  • Second bumps a bunch of eslint related dev dependencies to fix the lint task.
  • The eslint bump causes some new lint errors as some defaults changed. It's pretty minimal (bunch of constructors on new lines) so this commit applies those fixes so that the project passes linting.
  • Last commit removes Windows and MacOS from the CI matrix as I discovered they were never actually running :(

That last one I ran into while working on something else. The CI for this project was missing this line in the workflow:

runs-on: ${{ matrix.os }}

It was hard coded to ubuntu-latest so our "Windows" and "MacOS" builds were really just running the same tests as Linux 3x. After fixing the lint issue and the run-on clause, I tried out a real matrix with those OS's and as expected it failed: https://github.com/sehrope/node-postgres/actions/runs/7427371357/job/20212934334

This PR does not add back either of them yet as it's kind of complicated to set up Windows to pretend to a be a *nix environment. And from what I've seen so far, the MacOS runners are really slow to setup docker etc.

Figure we should at least get the "remove these dupes" piece in first and can have separate PRs to add those OS platforms into the matrix. May make sense to only run them on a smaller set of node versions as well.

Not sure how anybody could actually review the yarn lock piece of the version bumps. If it's easier, I can pull that out so you can run the install locally and generate the pinned versions.

@sehrope sehrope marked this pull request as draft January 5, 2024 23:02
@sehrope
Copy link
Contributor Author

sehrope commented Jan 5, 2024

Changing to draft until I figure out why this is failing again. I thought this was passing after the update but apparently it's hanging on something else.

…g linux

Removes the windows and macos matrix from the CI workflow as they were never actually setting
the OS. Both were running against the "ubuntu-latest" OS. Trying to actually use them would
not work either as neither windows or macos is supported for service containers. A different
means will be needed to test on those platforms. Until that's done, this removes those from
the matrix as we were simply running the same thing 3x for the same node versions.
@sehrope
Copy link
Contributor Author

sehrope commented Jan 5, 2024

Looks like it was caused by me adding v20 into the matrix. Only that one failed on the pg-native tests. I removed it from the matrix (so it's the same versions as before the PR) and now it's passing.

@sehrope sehrope marked this pull request as ready for review January 5, 2024 23:14
@charmander
Copy link
Collaborator

Related: #3110

@sehrope
Copy link
Contributor Author

sehrope commented Jan 10, 2024

Ah! I didn't notice that existing one for fixing the eslint stuff.

We should limit dependabot to less open PRs (like maybe just one). It's a lot noise to signal and ends up taking up the entire first screen worth of the PR list...

@brianc
Copy link
Owner

brianc commented Mar 5, 2024

well dang i totally missed this PR. Sorry. I have an update I'm going to post on the documentation soon, but I am going to be switching to full time (well probably part time, but my only form of "employment") work on node-postgres soon. May 1st is when I plan on it. So I'll be a lot more responsive & will come out w/ some tutorials and so on...have a bunch of plans. In the mean time I'll still struggle to keep up, but its about to turn around. 😄 Appreciate the help!

@brianc brianc merged commit b4bfd63 into brianc:master Mar 5, 2024
6 checks passed
@sehrope
Copy link
Contributor Author

sehrope commented Mar 5, 2024

Thanks! CI on master is green now ✔️ https://github.com/brianc/node-postgres/actions/runs/8162362778

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