Switch to GitHub Actions #556

andyundso · 2024-04-23T20:30:55Z

This PR switches the pipeline for tiny_tds from CircleCI to GitHub Actions.

Functionality-wise, they should be equivalent, although I made a couple of additions:

The ports are compiled in one step for all the Linux jobs, similar to how we do this for Windows.
Test summary is displayed for each job, e.g. here.
The built gem versions for Windows can also be downloaded from that page.
I also test the installation of the gem on Mac with M1 processors. This was something I added once to CircleCI for Add /opt/homebrew to search path on Apple Silicon #545, only to realise it is only available for paid organizations.

Open questions / points from my side:

Currently, the installation on Windows for MSSQL 2017 is broken, so I commented it out in the matrix and reported an issue upstream (MSSQL 2017 on Windows does not work potatoqualitee/mssqlsuite#27). Worst case we could re-activate the solution we used for CircleCI, as I did not find another action that can install multiple versions of SQL server and on different platforms. I could also fork (or re-implemented) a similar solution on my own time.
docker-composee.yml currently uses the CircleCI Ruby image. I like having this compose file around for a quick start into tiny tds development, but I would suggest to switch the official Ruby images from Docker Hub.
There are a bunch of scripts we no longer would need (like setup_cimgruby_dev).

This is a test specifically for rails-sqlserver#545, which we were unable to run on CircleCI since we did not have paid runners.

aharpervc

Awesome! Mostly just offering pedantic comments/reflections...

aharpervc · 2024-04-23T20:36:50Z

.github/workflows/ci.yml

+      - name: Build gem
+        shell: bash
+        run: bundle exec rake gem:for_platform[${{ matrix.platform }}]
+      - name: Move gems into separate directory before persisting


Is this step still required? I vaguely recall difficulties on CircleCI, not sure if that'll still be a problem here...

true, CircleCI did not support wildcard pattern, that's why we added this step.

I addressed removed this with 1d9a7c9.

aharpervc · 2024-04-23T20:38:31Z

.github/workflows/ci.yml

+      - name: Download precompiled gem
+        uses: actions/download-artifact@v4
+        with:
+          name: gem-x64-mingw32


I guess we can't/won't parameterize this for the install task, because we're matching the VM?

technically we are matching the Ruby version, everything below 3.0 is mingw32, 3.1 and newer is ucrt.

we probably could make a parameter out of it, or combine it with matrix, but to keep it simple I would suggest to spell it out explicitly for now.

aharpervc · 2024-04-23T20:39:26Z

.github/workflows/ci.yml

+          ruby -e "require 'tiny_tds'; puts TinyTds::Gem.root_path"
+          exit $LASTEXITCODE


I recommend splitting this to it's own step, for better clarity on failure

addressed with dc3b1e4.

aharpervc · 2024-04-23T20:40:24Z

.github/workflows/ci.yml

+          - 2.7
+          - '3.0'
+
+    name: Test on Windows (MingW)


I think for PR check clarity, we'll benefit from seeing the ruby version (and maybe sql server version) directly in the job name. Probably same comment for all jobs as applicable. Also, I'm big fan of consistency across jobs, so maybe something like test-$os (ruby $ruby_version), build- ..., etc. Looking at the list here: https://github.com/andyundso/tiny_tds/actions/runs/8806805186/job/24172426029

I added a suggestion with 0b12511.

aharpervc · 2024-04-23T20:41:01Z

.github/workflows/ci.yml

+          Copy-Item -Path ".\tmp\gems\tiny_tds-$gemVersion-$rubyArchitecture\ports" -Destination "." -Recurse
+
+      - name: Setup MSSQL
+        uses: potatoqualitee/[email protected]


Are we able to use service containers here instead?

fwiw, there's also this https://github.com/ankane/setup-sqlserver/

Linux service containers are not available on Windows for GitHub Actions. On Linux, the referenced action will use containers.

re: the action from ankane: would also work, also misses support for MSSQL 2017.

aharpervc · 2024-04-23T20:48:35Z

.github/workflows/ci.yml

+          & sqlcmd -S localhost -U sa -P "c0MplicatedP@ssword" -i ./test/sql/db-create.sql
+          & sqlcmd -S localhost -U sa -P "c0MplicatedP@ssword" -i ./test/sql/db-login.sql
+
+      - name: Install toxiproxy-server


Possible to cache this?

... maybe?

I think it's just an executable for Windows, which we could cache. Chocolatey is just comfortable to get it running quickly. Installation also only takes about 1min, compared to MSSQL (~4min) and Ruby (~3min) it is "fast".

aharpervc · 2024-04-23T20:49:35Z

.github/workflows/ci.yml

+        uses: test-summary/action@v2
+        with:
+          paths: "test/reports/TEST-*.xml"
+        if: always()


I'm curious, how do test failures show up here?

I made a branch with a test that fails intentionally. here is the result: https://github.com/andyundso/tiny_tds/actions/runs/8900358242

here is also a screenshot:

aharpervc · 2024-04-23T20:53:42Z

.github/workflows/ci.yml

@@ -0,0 +1,443 @@
+name: Build and test gem


Also with the question(s) of how checks show up in the PR UI, this might be better as name: ci

addressed with 6c4e643.

aharpervc · 2024-04-23T20:54:24Z

.github/workflows/ci.yml

+name: Build and test gem
+
+on:
+  push:


We might want to limit push to master, keeping the rest of the on's as you have it. Another way to say it, I don't think pushes to branches w/o PR's needs to run this CI workflow...

if you fork the repository and want to run the actions, you would have to open a PR. therefore, I added this on: push, because it makes development in forks easier.

aharpervc · 2024-04-23T20:55:32Z

.github/workflows/ci.yml

+      - name: Install gems
+        shell: bash
+        run: bundle install
+      - name: Write used versions into file


My fake undiagnosed OCD demands more newlines for this job, same as you have the others 😆

addressed with e994b63.

bf4

given the complexity of this PR, it might be better to begin adding a GitHub ci action and then testing extending it before removing the circle ci config

bf4 · 2024-04-30T03:34:38Z

quick and dirty brainless importer didn't work. but now that there's an action on main with workflow dispatch, you can test it in a PR

example failures https://github.com/rails-sqlserver/tiny_tds/actions/runs/8888886154

andyundso

@aharpervc I think I addressed all your feedback.

I would adjust the development docker compose in a second PR, to not make this one even more bloated. :)

andyundso · 2024-04-30T18:54:41Z

.github/workflows/ci.yml

+      - name: Build gem
+        shell: bash
+        run: bundle exec rake gem:for_platform[${{ matrix.platform }}]
+      - name: Move gems into separate directory before persisting


true, CircleCI did not support wildcard pattern, that's why we added this step.

I addressed removed this with 1d9a7c9.

andyundso · 2024-04-30T18:55:19Z

.github/workflows/ci.yml

@@ -0,0 +1,443 @@
+name: Build and test gem


addressed with 6c4e643.

andyundso · 2024-04-30T18:56:19Z

.github/workflows/ci.yml

+name: Build and test gem
+
+on:
+  push:


if you fork the repository and want to run the actions, you would have to open a PR. therefore, I added this on: push, because it makes development in forks easier.

andyundso · 2024-04-30T18:57:31Z

.github/workflows/ci.yml

+      - name: Install gems
+        shell: bash
+        run: bundle install
+      - name: Write used versions into file


addressed with e994b63.

andyundso · 2024-04-30T18:59:11Z

.github/workflows/ci.yml

+          - 2.7
+          - '3.0'


addressed with c7ae011.

andyundso · 2024-04-30T19:12:46Z

.github/workflows/ci.yml

+          & sqlcmd -S localhost -U sa -P "c0MplicatedP@ssword" -i ./test/sql/db-create.sql
+          & sqlcmd -S localhost -U sa -P "c0MplicatedP@ssword" -i ./test/sql/db-login.sql
+
+      - name: Install toxiproxy-server


... maybe?

I think it's just an executable for Windows, which we could cache. Chocolatey is just comfortable to get it running quickly. Installation also only takes about 1min, compared to MSSQL (~4min) and Ruby (~3min) it is "fast".

andyundso · 2024-04-30T19:20:13Z

.github/workflows/ci.yml

+        uses: test-summary/action@v2
+        with:
+          paths: "test/reports/TEST-*.xml"
+        if: always()


I made a branch with a test that fails intentionally. here is the result: https://github.com/andyundso/tiny_tds/actions/runs/8900358242

here is also a screenshot:

andyundso · 2024-04-30T19:20:47Z

.github/workflows/ci.yml

+
+      - uses: ruby/setup-ruby@v1
+        with:
+          ruby-version: 3.3


we only need to compile the ports once for all the Linux jobs, so no matrix (aka parallel jobs) needed.

andyundso · 2024-04-30T19:21:45Z

.github/workflows/ci.yml

+    steps:
+      - uses: actions/checkout@v4
+
+      - name: Install FreeTDS


apparently possible, but looks messy:
https://stackoverflow.com/a/65056232

andyundso · 2024-04-30T19:23:34Z

.github/workflows/ci.yml

+
+      - uses: ruby/setup-ruby@v1
+        with:
+          ruby-version: ${{ matrix.ruby-version }}


I think was just an oversight. I added it now to all jobs: 49c557d.

aharpervc

LGTM

bf4

Great to see this green.

If it's ok to merge I'm happy to merge it.

Then I'll remove the not-actually-working circleci->gh-actions import I added whilst trying to be helpful (.github/workflows/test.yml and .github/actions/install-ruby-windows/action.yml) and if y'all have admin to remove the circleci check we can remove that else put in a no-op config till we can get Ken to remove it

andyundso added 5 commits April 14, 2024 22:42

Remove CircleCI configuration

b00d773

Setup GitHub Actions

2416b1d

Setup test summary

7dcd4fa

Test installation on M1 Mac

b6ca788

This is a test specifically for rails-sqlserver#545, which we were unable to run on CircleCI since we did not have paid runners.

Disable tests with Windows and MSSQL 2017 (for now)

780bff4

andyundso requested a review from aharpervc April 23, 2024 20:44

aharpervc reviewed Apr 23, 2024

View reviewed changes

aharpervc assigned andyundso Apr 23, 2024

bf4 reviewed Apr 26, 2024

View reviewed changes

bf4 mentioned this pull request Apr 30, 2024

ci: import circlei via gh-actions-importer #558

Merged

andyundso added 7 commits April 30, 2024 20:44

Remove moving built gems into separate directory

1d9a7c9

Adjust name

6c4e643

Add more space

e994b63

Quote all Ruby version values

c7ae011

Move "Test if TinyTDS loads" into separate steps

dc3b1e4

Harmonize job names

0b12511

Setup bundler cache for all Ruby installation jobs

49c557d

andyundso commented Apr 30, 2024

View reviewed changes

andyundso requested a review from aharpervc April 30, 2024 19:37

andyundso marked this pull request as ready for review April 30, 2024 19:41

Disable fail-fast

6a33e07

aharpervc approved these changes May 9, 2024

View reviewed changes

bf4 approved these changes May 9, 2024

View reviewed changes

andyundso merged commit 13acf23 into rails-sqlserver:master May 10, 2024
44 of 45 checks passed

bf4 mentioned this pull request May 10, 2024

ci: remove unused actions #559

Merged

		ruby -e "require 'tiny_tds'; puts TinyTds::Gem.root_path"
		exit $LASTEXITCODE

Switch to GitHub Actions #556

Switch to GitHub Actions #556

Conversation

andyundso commented Apr 23, 2024 • edited Loading

aharpervc 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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bf4 left a comment

Choose a reason for hiding this comment

bf4 commented Apr 30, 2024

andyundso 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

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

Choose a reason for hiding this comment

aharpervc left a comment

Choose a reason for hiding this comment

bf4 left a comment

Choose a reason for hiding this comment

andyundso commented Apr 23, 2024 •

edited

Loading