rails-sqlserver / tiny_tds

TinyTDS - Simple and fast FreeTDS bindings for Ruby using DB-Library.
Other
607 stars 189 forks source link

CI improvements #525

Closed andyundso closed 1 year ago

andyundso commented 1 year ago

Background

I read through @aharpervc feedback on my PR for the Ruby 3.0 support (#524). He asked to implement three features for the CI noted here.

The issue I see with the current CI is that each Windows build step compiles the gem for itself. I assume this means that the compiled version works exactly for one Ruby version. So in order to test it on Windows, you would need to download the artifact using your Ruby version and architecture that you want to test. However, in the final fat gem, the needed binaries are provided for all the supported Ruby versions.

Second issue I see is the build time on Windows. Although the time would go down when using a cache, the gem was only build on Windows, but not tested.

So the main idea behind those pipeline changes are:

I originally had another step in mind that runs after all the tests passed and will build the actual gem, but this proofed to not work very well with cache, as it always recompiled the tiny_tds code, which is the one thing I didn't wanted to happen.

Preparation

There were a couple of changes necessary to enable this workflow. Some are the same as in #524.

Windows setup

I would say the Windows setup is quite simple:

There is one test named raises TinyTds exception with dead connection network failure that didn't work. I was able to reproduce this on my Windows machine, but didn't really see how I can fix that, so I skip it on Windows for now as it seems to work fine on Linux.

Performance

Previously, the Windows builds had about 30min to complete. I checked several runs with this pipeline to see how it performs now.

So totalling at between 14 and 22 minutes, depending if the cache is present.

... speaking of caches

I added caches for the following things:

Potentially we can also cache:

Other changes

Sorry for the very long PR description, I wanted to be explicit what I built and thought as there are quite some changes.

andyundso commented 1 year ago

ping @aharpervc :)

andyundso commented 1 year ago

glad to hear that the changes generally look good to you.

I updated the PR description now to better reflect what's actually in the PR, as some thing changed with your initial review.

andyundso commented 1 year ago

since it's been two weeks, I would like to ask if you could review the PR so can get it merged (@aharpervc @bvogelzang @larskanis)?

andyundso commented 1 year ago

sorry to annoy again, but Ruby 2.7 goes end of life in about a month and it would be nice if I could start working on the support for Ruby 3, but those changes need to get merged first (@aharpervc @larskanis @bvogelzang) (not sure why Github removed the review requests , i didn't do it intentionally ..)

aharpervc commented 1 year ago

@andyundso I've approved this PR & added you as a collaborator, so you should be able to merge when you're ready

andyundso commented 1 year ago

@andyundso I've approved this PR & added you as a collaborator, so you should be able to merge when you're ready

great, thanks for the trust! will probably merge on Sunday so I can get back to #524 afterwards.