rails-sqlserver / tiny_tds

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

[Experimental] CircleCI #514

Closed bryanwieg closed 1 year ago

bryanwieg commented 2 years ago

Extention of work by aharpervc here: https://github.com/veracross/tiny_tds/pull/1

Results here: https://app.circleci.com/pipelines/github/bryanwieg/tiny_tds?branch=circleci&filter=all

aharpervc commented 2 years ago

Great progress, this is huge. Next steps:

  1. restore the ruby version parameterization (I'm thinking major.minor if possible, and let the patch version float. Not sure if this will be possible if we need the FULL version to template a url)
  2. restore the linux job(s)
  3. make sure we're able to use all reasonable caching to make things as speedy as possible

That'd be enough for this PR. However I'd like to do more at some point, but it could be a second PR (by you or someone else if you don't have time)

  1. investigate if we can make a macOS job
  2. set up integration with CircleCI's test reporting: https://circleci.com/docs/2.0/collect-test-data/
  3. set up integration with CircleCI's artifacts collection: https://circleci.com/docs/2.0/artifacts/ (this way, people observing PR's can grab release gem instead of building from source)

Also we should consider restructuring the jobs workflow(s). For example, we might benefit from having "test" be separate from "build", and then require test to succeed before build runs. That probably isn't critical though.

@bvogelzang any thoughts?

bryanwieg commented 2 years ago

This looks good, first three points look good. I need to clean things up.

I was wondering about the tests. In appveyor, they have a section spinning up SQL Server and executing some rake tests. I suppose there are also tests for Linux- with docker and so on. Do these need to be implemented in circleci as well?

As a side note, for a later time, as I work on this, I can see other things that could be updated. Such as perhaps some components for sql server 2019.

I unfortunately do not have a mac, so I cannot help with that.

bvogelzang commented 2 years ago

Wow. Amazing work đź‘Ź

@aharpervc I agree with all of this

investigate if we can make a macOS job

This should be relatively straightforward and really similar to linux. The harder part will be getting the mssql-server-linux-tinytds docker image running on the MacOS executor https://support.circleci.com/hc/en-us/articles/360045029591-Can-I-use-Docker-within-the-macOS-executor-

For example, we might benefit from having "test" be separate from "build", and then require test to succeed before build runs.

Yes, definitely something we should do but agree these are future goals. Not anything that needs to be accomplished within this PR. It would also be nice to add automated releases as part of that work.

bryanwieg commented 2 years ago

I am working on this today. Do we want ruby 2.5 in the build matrix, as it is already past eol?

Any thoughts?

bryanwieg commented 2 years ago

Also, as I moved on to looking at how the windows tests would work, I noticved some things...

If I read this right, CircleCI Windows 2022 image does have sql server 2019 developer pre-installed. But I can see the old appveyor appears to have had a custom image with 3 different (older) sql server instances to test against.

At the moment I can see that testing the windows port may take some extra consideration. At least the Windows matrix is now working with two Ruby versions.

I'm going to move back to restoring the Linux build. The tests there should be much easier with docker.

But I welcome any thoughts about how to set up testing on the windows side.

aharpervc commented 2 years ago

I am working on this today. Do we want ruby 2.5 in the build matrix, as it is already past eol?

I would prefer to keep it on the list for the time being. A later PR could remove support for 2.5 by requiring 2.6+ in the gemspec and removing 2.5 from the CI list. But I'd prefer that be a separate change and a gem version bump.

bryanwieg commented 2 years ago

makes sense, thank you

aharpervc commented 2 years ago

If I read this right, CircleCI Windows 2022 image does have sql server 2019 developer pre-installed. But I can see the old appveyor appears to have had a custom image with 3 different (older) sql server instances to test against.

IIRC the sql server image used for testing is a mostly empty image, and is available to make running SQL Server easier. I'm not clear on how easily we can run Linux docker image on a Windows host in CircleCI (considering you can do this apart from CI you'd think it'd be possible, but whatever).

BTW, I made the veracross/circleci-ruby-freetds image to pre-build FreeTDS for back in the days when the source code was commonly available via a flaky FTP server that'd go down a lot and the git repo didn't have version tags. More recent version of FreeTDS do have version tags on GitHub: https://github.com/FreeTDS/freetds/tags. Therefore rather than using that custom base image, we can probably build FreeTDS either via the rake task or by cloning the repo and switching to a supported/tested tag, or the most recent release, etc. An example of that is here: https://github.com/veracross/ruby-app-base/blob/7a19152abb0a5c0d6526a405dcb2d163be86c2b9/Dockerfile#L19-L31 (using the built-in rake task here is likely preferable, even if we have to manually curl a tarball first or something)

e: presumably the native builds can be cached in CircleCI as well, so each test run should be fairly speedy when it's all set up

bryanwieg commented 2 years ago

As far as I know, it would not be possible to run a linux container on windows without hyperv support and docker desktop. Which I highly doubt is possible in circleci. MS official prebuilt SQL for Windows Containers were discontinued long ago (they were also >12GB large). Even if we made some, they would be very big.

This means, as far as what circleci does provide for windows SQL Server, it would only be SQL Server 2019. Which I think we could use for testing. But I see no way to test older versions of SQL server without installing them in the CI run, which would take TON of ci minutes.

aharpervc commented 2 years ago

I agree, using the built-in SQL Server instance for the Windows jobs is probably the best option for now. I don't view not being able to parameterize the SQL Server version on Windows as a blocker here at all. I don't even think we absolutely need to parameterize that for Linux per se, and we should be able to use mcr.microsoft.com/mssql/server:2019-latest for that accessory image in the Linux jobs so at least it's the same between OS jobs.

Then for all the jobs it'll just be a matter of making sure we're able to run whatever scripts we need ahead of time to prepare that SQL Server instance for the automated tests, ie, these scripts: https://github.com/rails-sqlserver/docker-images/tree/master/mssql-server-linux-tinytds/tinytds

bryanwieg commented 2 years ago

Sounds good. Just to check for sure tough, we are saying only SQL Server 2019 for both windows and Linux is good?

aharpervc commented 2 years ago

Sounds good. Just to check for sure tough, we are saying only SQL Server 2019 for both windows and Linux is good?

I think so, yes. We could probably set up additional on a per-version basis for SQL Server for Linux, but it doesn't seem critical right now. It might become critical in the future, but not necessary for this PR.

Although it probably wouldn't be terribly difficult to support multiple versions. For example, if the CI jobs were windows_mssql-latest, linux_mssql-2019 and linux_mssql-2017 (with reference to the corresponding SQL Server docker image tags) that'd do a lot. Then you can imagine the level of effort to add a CI job for SQL Server 2022 when that docker image gets tagged. (Then we'll have a clear answer when people come here and post a question "hey, does TinyTDS support SQL Server 2022????")

aharpervc commented 2 years ago

We probably won't be this lucky, but I am a bit curious if the setup_remote_docker feature would give a Linux docker connection if run from a Windows job? 🤔

If it actually worked that way, that'd be ideal, since the Ruby test code on Windows could run SQL Server on Linux via a container, and we wouldn't need to rely on the pre-installed SQL Server for Windows instance.

(Probably won't be that lucky though....)

bryanwieg commented 2 years ago

I'm just posting this here so it is not forgotten. I don't know know when (or if it a good idea to use these) I can get around to this. Windows CI currently works as is and I'm trying to control scope creep at this point. Current on the Linux side.

Windows Docker SQL Containers SQL 2012-2019.

https://hub.docker.com/r/cagrin/mssql-server-ltsc2022

bryanwieg commented 2 years ago

@bvogelzang, regarding the tiny_tds circleci pipeline build...

I know its been a few months since I was able to resume this- I have a question please.. I'm pinging you because I see from a past mr that you may be familiar with some of the code involved in my issue.

For example: https://github.com/bryanwieg/tiny_tds/commit/10433d8fcf268f4278e8a3fef042ca176db38d39

I have linux working now in circleci (recall windows was already completed). It builds and runs the tests.

However, two tests are failing, blocking me. Both tests work, they just fail the assertion. Both have to do with toxiproxy tests. One fails with an error returned different than one that was expected. The other test fails expecting an error but none was thrown.

https://app.circleci.com/pipelines/github/bryanwieg/tiny_tds/72/workflows/aa995130-9fa7-40a5-8060-295e8a715c9a/jobs/91

1) Failure:
With in-valid options#test_0008_raises TinyTds exception with dead connection network failure [/home/circleci/repo/test/client_test.rb:168]:
Expected: 20047
  Actual: 20003

2) Failure:
With in-valid options#test_0009_raises TinyTds exception with login timeout [/home/circleci/repo/test/client_test.rb:187]:
expected a TinyTds::Error but none happened

Could you please take a look? I was able to replicate the problem using local docker containers for development.

I spent considerable time backdating versions of everything to match past builds on tiny_tds where these tests passed. Such as SQL Server 2017 GA, free_tds, and other gems. But I cannot get past these assersions. I don't know if it's a build environment issue, or if something else has changed regarding the original issues and the code itself needs to be updated.

You can use these general steps to get a dev container up to see the issue on your own machine.

  1. it's all in docker-compose: docker-compose up
  2. attach to the cimg/ruby container: docker container exec -it tiny_tds-cimgruby-1 /bin/bash
  3. build the dependencies: . ~/repo/test/bin/setup_cimg_dev.sh
  4. build the gem and run the tests: sudo -E bundle exec rake

Thanks for your time.

bryanwieg commented 2 years ago

@aharpervc, regarding the above post- in the mean time, I have disabled those two tests and moved forward.

In the latest pipeline, both windows and linux successfully build, and linux will successfully test. Both on all Ruby versions 2.5.x to 2.7.x.

https://app.circleci.com/pipelines/github/bryanwieg/tiny_tds/74/workflows/7d2bf856-c96a-4752-8a88-1ebfd2ae5dd6

Was there anything else that should be done in this particular pull request? As I go along, I see all sorts of things that could be done, but I'm trying to keep the scope under control so that we can at least make reasonable baby steps. Also, I could use a second set of eyes to see if anything is missing.

Thanks, Bryan

bryanwieg commented 2 years ago

thank you, this is exactly what I needed. I had done a lot to just get it to work. I have overlooked a lot of these things, I'll work on them

bryanwieg commented 2 years ago

I pinned versions of some things back to the last time pipelines ran, just to get it running in a seeming consistent way as it did before. Even updating SQL seemed like a strech. I was planning to leave the real updating of dependencies to another mr.

bryanwieg commented 2 years ago

@aharpervc, there is a pipeline running now.. assuming it all runs, the pr is ready for another review :)

bryanwieg commented 2 years ago

Hope you guys are all doing well. Just a reminder that the pr is still up whenever you have time.

bryanwieg commented 2 years ago

crazy timing, circleci just sent out an email about a plan for open source projects: https://circleci.com/open-source/

bvogelzang commented 2 years ago

It looks like option number 3 will work. Here is the commit to get the toxiproxy tests passing: https://github.com/bvogelzang/tiny_tds/commit/b83445c3edb48c186ab0e4fc1b4bb1bc57f21508. Hope that helps!

bryanwieg commented 2 years ago

sorry for the delay.

thanks for replicating this. The tests connecting to the sql server container work fine. I wonder why then they do not connect to toxiproxy container. So this is helpful, I was so convinced it was the tests themselves I never tried just installing the toxyproxy package to verify that.

I'll try to dig into it more. If connecting to the sql server container works, I can not see why there is not a way to get it connecting to the toxiproxy container also. The docs say that the tests are running from the first container cimg/ruby and from there connect to the other containers.

examples:

I'll keep digging. I feel like I would like to use the toxiproxy container, but I may yield and give in and just install the package if it does not work.

bvogelzang commented 2 years ago

I think things are able to connect to the toxiproxy container on the default exposed port of 8474, the problem happens when you need to connect to the 1234 port to run the tests. We have no way of exposing that port on the container in a multi container environment as far as I know (without creating a custom image).

bryanwieg commented 2 years ago

ahhhhh, yes, good point. I'll be sure to look into that.

bryanwieg commented 2 years ago

based on @bvogelzang insights, I was able to find the problem and get it to work in docker and using docker-compose.

To do it in circleci would require using a linux machine environment and docker-compose.

I am going to present a version of this pr that uses this method. Two of the benefits being that you could develop locally all using containers, and using the exact same build environments locally as well an in the ci pipeline.

bryanwieg commented 2 years ago

@aharpervc and @bvogelzang, well it is ready for a look, all tests passing!

bryanwieg commented 2 years ago

Hello @aharpervc and @bvogelzang, hope you are doing well, just wondering if you had any thoughts on this so far.

bryanwieg commented 1 year ago

Hey @aharpervc , @bvogelzang , hope everyone is doing well! Let me know if there is anything I can do to nudge this forwards

ecentell-CPF commented 1 year ago

I'd also like to nudge this forward as it may allow PR503 to be merged.

aharpervc commented 1 year ago

I haven't forgotten; just buried with other things. I have a few misc questions/thoughts.

  1. We need to figure out the circleci org. I'm assuming it'd be preferable for it to be named rails-sqlserver like the GH org here
  2. There's still a lot of "manual setup" going on in these steps, such as docker exec cimg_ruby bash -c 'sudo -E ./test/bin/install-freetds.sh' and manually downloading/installing ruby for windows. If that's how it has to be, then so be it I guess. And we can always iterate later (the priority is definitely getting some functional cross platform CI operational...)
  3. Remind me why we have to do "docker exec cimg_ruby bash ..." as an approach generally at all?
  4. I'd like input from any other contributors that are around cc @wpolicarpo
bryanwieg commented 1 year ago
  1. We need to figure out the circleci org. I'm assuming it'd be preferable for it to be named rails-sqlserver like the GH org here

This is like- the last thing, that I know of, we are waiting for. I don't know how it needs to be done. For example, does a maintainer need to set api keys for circleci to hook into the repo? I think circleci wants api keys

  1. There's still a lot of "manual setup" going on in these steps, such as docker exec cimg_ruby bash -c 'sudo -E ./test/bin/install-freetds.sh' and manually downloading/installing ruby for windows. If that's how it has to be, then so be it I guess. And we can always iterate later (the priority is definitely getting some functional cross platform CI operational...)

manually downloading and installing ruby is required for it to build in windows due to various limitations

  1. Remind me why we have to do "docker exec cimg_ruby bash ..." as an approach generally at all?

what you are seeing is circleci (running from the linux executer vm) hooking to into the cimg ruby container and building the environment

the reason we are using containers on top of a full linux vm is to accommodate toxiproxi and localhost networking, which required a complete native docker environment

this approach has the byproduct of allowing the entire application to be developed locally in a container (via docker-compose up) identically to how it would be built in the ci

  1. I'd like input from any other contributors that are around cc @wpolicarpo
aharpervc commented 1 year ago

This is like- the last thing, that I know of, we are waiting for. I don't know how it needs to be done. For example, does a maintainer need to set api keys for circleci to hook into the repo? I think circleci wants api keys

Hmm, maybe. This is an open source repo so you wouldn't think there'd need to be any secrets. Maybe it's a matter of approval at the org level or something?

manually downloading and installing ruby is required for it to build in windows due to various limitations

"Manual" is the question here, not the step itself. I have a vague memory that one of us tried to use scoop or chocolately and it didn't work?

the reason we are using containers on top of a full linux vm is to accommodate toxiproxi and localhost networking, which required a complete native docker environment this approach has the byproduct of allowing the entire application to be developed locally in a container (via docker-compose up) identically to how it would be built in the ci

That makes sense, thanks. I wonder if it's be less confusing (or less work) to install toxiproxy some other way, rather than trying to maintain an identical setup to local development. Regardless, I don't think this question needs to block merging this PR, since if we figure out a better way in the future at least we'll have had functional CI all that time

bvogelzang commented 1 year ago

I wonder if it's be less confusing (or less work) to install toxiproxy some other way

I would vote to go this route as I suggested in my comment above:

It looks like option number 3 will work. Here is the commit to get the toxiproxy tests passing: bvogelzang@b83445c. Hope that helps!

Toxiproxy install/config becomes two steps in .circleci/config.yml instead of using the container

      - run:
          name: install toxiproxy
          command: |
            wget -O toxiproxy-2.1.4.deb https://github.com/Shopify/toxiproxy/releases/download/v2.1.4/toxiproxy_2.1.4_amd64.deb
            sudo dpkg -i toxiproxy-2.1.4.deb
      - run:
          name: start toxiproxy
          background: true
          command: |
            sudo toxiproxy-server -port 8474 -host localhost

While we would lose the ability for local development/tests to run as it is in CI I think this is an appropriate trade-off to reduce complexity as opposed to hiding CI configuration behind various shell scripts. Just my opinion. However, I agree this doesn't need to block merging this PR.

bryanwieg commented 1 year ago

"Manual" is the question here, not the step itself. I have a vague memory that one of us tried to use scoop or chocolately and it didn't work?

that is correct, quite a lot of time was spent on chocolatey and scoop. in the end those would repackage the rubydev and msys2 tools separately and install them in such a way that the dependencies such as openssl and libiconv would not build.

finally switched to installing the native rubydev with msys2 bundle using its own installer and it 'just worked'

I understand about the containers thing, it was a suggestion. It has helped me quite a lot to be able to do all this locally because circleci will run out of minutes rather quickly if you try pushing many, many small changes to figure stuff out. but I don't develop on this project as you all do. that said, I am thankful that it can squeeze in just so that this can move forward.

what I am referring to when trying to add a repo to a project in circleci is this:

image

bryanwieg commented 1 year ago

hello all, just wanted to check in on the tinytds circleci pr. see if we are waiting on anything from me

ecentell-CPF commented 1 year ago

I'd also like to nudge this forward as it may allow PR509 to be merged and resolve ISSUE503.

ecentell-CPF commented 1 year ago

Any update on this?

bryanwieg commented 1 year ago

I haven’t heard anything since the last comments, unfortunately.

ecentell-CPF commented 1 year ago

Bump, another month Can any devs weigh in on this?

bryanwieg commented 1 year ago

I am ready to keep working on it if the project is still maintained. But since it seems to have slowed down, we have had to look at forking or removing this dependency.

aharpervc commented 1 year ago

I am ready to keep working on it if the project is still maintained. But since it seems to have slowed down, we have had to look at forking or removing this dependency.

It's "maintained" in the sense of accepting volunteer contributions like you have done here in this PR, which benefit everyone. I don't think anyone has blocking complaints about the code itself, but there is the open question of the CircleCI config.

Regarding that...

what I am referring to when trying to add a repo to a project in circleci is this:

Is there an option for GitHub on that list? Your screenshot there shows GitLab

bryanwieg commented 1 year ago

thanks @aharpervc, my mistake regarding gitlab. let me check that now...

bryanwieg commented 1 year ago

i apologize, the point regarding my gitlab/github mistake pointed me down a path that ultimately answered, i think, how to do it.

i believe once this is merged, a circleci org will automatically be created and associated with the org owners of the repo here in github- that is to say rails-sqlserver. the link i've supplied -i believe- should point to it's location in circleci based on the documentation.

docs would be here-> https://circleci.com/docs/status-badges/#generating-a-status-badge

bryanwieg commented 1 year ago

my org checked just recently on this as a matter of fact. if we can keep this moving and updated to ruby 3.0, it will be very helpful to us and allow us to help maintain it as well. thank you for your help

aharpervc commented 1 year ago

i believe once this is merged, a circleci org will automatically be created and associated with the org owners of the repo here in github- that is to say rails-sqlserver. the link i've supplied -i believe- should point to it's location in circleci based on the documentation.

It's possible that we'll have to manually set up the project in CircleCI. I just clicked the button to (apparently?) authorize that integration for the rails-sqlserver GH org. It doesn't appear that the integration can be previewed with the new config file coming from a fork, but that should be point & click after merging this, if my understanding is correct.

bryanwieg commented 1 year ago

thank you @aharpervc :)

what would be the next steps? do we still need an approval from @bvogelzang

bryanwieg commented 1 year ago

is there a preference on cleaning up the commit history or commit message?

aharpervc commented 1 year ago

is there a preference on cleaning up the commit history or commit message?

In this case the intermediate branch commits don't add anything useful to the master history, so I recommend squashing here on the branch first before merging

bryanwieg commented 1 year ago

squashed, and circleci is running on the local branch just to check one last time