-
Notifications
You must be signed in to change notification settings - Fork 622
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
CASSANDRA-19796 – cassandra-gocql-driver CI #1812
Conversation
@ribaraka I took your work and trying to pass the pipeline here, lets see how it goes ... You can just create PRs against my PR and when I merge it into this PR it will run the pipeline again. I think that PRs are not automatically run for non-maintaners at the moment. We might look into how to change that. |
@ribaraka we need to include 5.0.0 into this as well. It should be released quite soon. We should also bump Go versions. Please go over (1) and let me know what you think as well. |
Should we look into adding semantic versioning while refactoring CI? Correct me if I'm wrong, but so far the tags seem to be created manually? |
@hardikmadhu could you elaborate more what you mean by that? Should we look into adding semantic versioning while refactoring CI? |
@smiklosovic I've already updated the project's Go version to 1.21. If further updates are needed, should I upgrade it to the latest version, which is currently 1.23?
I have looked at all of the points in (1) and they all looks reasonable to me. Got a few questions so far:
|
what was your motivation to move it from 1.19 and 1.20 to 1.20 and 1.21? Why not 1.23? I am just asking why you bumped it like that. I am not too strong in Go so I would ask this: what are the official stable releases at this moment? What is the roadmap in general? I wonder what was the motivation to test it on both 1.19 and 1.20. I guess that at the time there was 1.20 as the lastest one and 1.19 one before so that made sense to the original author of that code. Also, are 1.23 and 1.19 compatible when it comes to gocql? What it means for users who will need to update their gocql dependency while they see we started to build it with 1.23? Will they need to update their Go infrastructure to 1.23 as well to run this? Anyway, this is indeed interesting discussion and I think we should have a separate discussion about that in JIRA (you are welcome to create an account there) https://issues.apache.org/ For now, I would left this untouched (1.19 and 1.20 might stay and we will target that in next tickets).
|
I'm not sure how these projects are usually structured. Also, I'm talking more CD than CI. I just thought I'd bring this up while we're working on this ticket. We could use something like this or this to automate releases. If I'm not wrong, currently the release tags are created manually. |
My main goal was to update the integration test runner by integrating the testcontainers-go library. I aimed to maintain the CI workflow as authentic as possible, awaiting further guidance from maintainers on the next steps.
As of now, the official stable releases of Go is 1.23 https://go.dev/dl/
As far as I know, the plan is to support the final version of the CQL protocol v5, include data types from the latest Cassandra versions, and clean up deprecated features.
If gocql begins using features or standard library changes that were introduced in Go 1.23, users who still run their infrastructure on Go 1.19 might encounter compilation errors or runtime issues. In that case, they would need to upgrade to Go 1.23 to use the latest version of gocql. Given the issue I described above (the need to upgrade from 1.19) and the test results, Go version 1.20 should be sufficient.
https://github.com/apache/cassandra-gocql-driver/blob/trunk/CONTRIBUTING.md
Maybe someone wants to try using testcontainers featute ASAP 😄 |
OK so I think that we should code against 1.20 and having 1.23 supported which is the latest one. Basically, test the oldest version possible and the newest one makes sense to me. Have you tried to bump it to 1.23 to see if it still works? Contributor guide is OK, just link that from the main readme or made it more visible so people do not need to look for that for too long. |
I upgraded the project’s Go version to 1.23 and added a Go version matrix (1.20-1.23) to the CI pipeline. The pipeline ran successfully for all versions except Go 1.20. Currently, I'm encountering a compilation error when trying to build Go 1.23 code with Go 1.20. When updating the Go version to 1.23 only in the CI workflow for this PR (while the project's Go version remains at 1.21), both versions 1.20 and 1.23 work fine. The current PR's Go version seems to strike a good balance across all these versions.
#1686 (comment) From the Go Release Policy :
@martin-sucha might have a good vision for the project’s go version. At least he suggested to bump the version up to 1.21.
OK, I will do it. |
Yesterday and today, I attempted to find a way to delay upgrading the project's Go version. I tried integrating the testcontainers library without updating the project version, but I kept encountering dependency errors. I could have addressed these dependency issues with workarounds and fixes, but honestly, it’s time-consuming and may not be worth the effort in the long run. I also switched to the most up-to-date version of the trunk branch and ran the tests using the current Go version of the project (1.13), but the build keeps failing. The error I encountered is:
These errors are related to features that were introduced in later Go versions, which means they won’t work when building the code with Go 1.13, as the methods don’t exist in that version of the Go standard library. Given these issues, I think it’s time to upgrade to a newer Go version instead of trying to maintain compatibility with the current version. |
Yeah, updating that to at least 1.20 makes sense. We are discussing about this in asf slack channel for drivers, would you mind to join there? Ask Dmytro how we did it and we could just repeat that for you too. It is good to have somebody to talk to in more "interactive" way. This is great work overall, I just need to let you know that we will more realistically merge this when we have a clean description how versioning / branching will work from now on etc. so we have something in hand to base our decisions on. I should write that document in Cassandra's wiki but that is still pending so please stay tuned and keep in touch, in the meanwhile try to figure out how to join to drivers channel. |
Worth noting that I'm planning on bumping the Go versions for the 1.7.0 release to remain consistent with the old sunset model that was used by gocql. That work is taking place in #1813. I want to change as little as possible for the 1.7.0 release so I'd prefer to avoid any conversion to testcontainers or any other changes there. |
sure we can wait if you think that would be better |
@smiklosovic, I've added a fix for the flaky tests.
I’ve submitted a PR for your review. Let me know if you have any feedback. |
@ribaraka something is broken, I think you need to update go to 1.23 in the matrix |
@smiklosovic done! re-push please |
d846611
to
6b6307e
Compare
@smiklosovic I've added documentation on how to run tests locally c9e43bd. The work on setting up Cassandra v5.0.0 is in progress. |
ok great, thank you, just put all stuff to your branch and ping me once done |
I've consolidated all the work into the #1780 PR |
6b6307e
to
12ba1c5
Compare
@absurdfarce @ribaraka this might go in as we are after 1.7.0? |
@ribaraka can you please resolve the conflicts after we released 1.7.0? @absurdfarce can you please cooperate on this with @ribaraka ? I wont be around for a while. |
Can we close this and focus on #1780 ? Or are the 2 PRs doing different things? |
done |
Yes, we can focus on #1780. #1812 has been duplicated, so everyone can see the GitHub action running successfully. Now that my workflow (in the #1780) has been approved by a maintainer and I can run it on this repo, there's no need to keep two PRs. upd1: After I resolved the conflicts in the PR and pushed the changes again, the workflow approval status reverted to pending. |
No description provided.