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

CASSANDRA-19796 – cassandra-gocql-driver CI #1812

Closed
wants to merge 9 commits into from

Conversation

smiklosovic
Copy link
Contributor

No description provided.

@smiklosovic
Copy link
Contributor Author

@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.

@smiklosovic
Copy link
Contributor Author

@smiklosovic
Copy link
Contributor Author

@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.

(1) https://issues.apache.org/jira/browse/CASSANDRA-19796?focusedCommentId=17879165&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-17879165

@hardikmadhu
Copy link

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?

@smiklosovic
Copy link
Contributor Author

@hardikmadhu could you elaborate more what you mean by that?

Should we look into adding semantic versioning while refactoring CI?

@ribaraka
Copy link

ribaraka commented Sep 4, 2024

We should also bump Go versions.

@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?

Please go over (1) and let me know what you think as well.

I have looked at all of the points in (1) and they all looks reasonable to me. Got a few questions so far:

  1. Where should I document the process of running integration tests locally? I'm thinking of including it in the contribution guide. Should this documentation be added in this PR?
  2. Should the fix for the flaky tests also be included in this PR?

@smiklosovic
Copy link
Contributor Author

smiklosovic commented Sep 4, 2024

@ribaraka

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).

  1. The best place would be in README.md,. What contribution guide do you have in mind? Can you show the link to that?
  2. Yes, why not? I don't see a reason why we should merge it separately.

@hardikmadhu
Copy link

hardikmadhu commented Sep 4, 2024

@hardikmadhu could you elaborate more what you mean by that?

@smiklosovic

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.

@ribaraka
Copy link

ribaraka commented Sep 4, 2024

@smiklosovic

what was your motivation to move it from 1.19 and 1.20 to 1.20 and 1.21? Why not 1.23?

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.
However, I moved away from Go 1.19 because a function introduced in Go 1.20 is not available in the earlier version. #1686 (comment)

what are the official stable releases at this moment?

As of now, the official stable releases of Go is 1.23 https://go.dev/dl/

What is the roadmap in general?

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.

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?

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.

  • What contribution guide do you have in mind? Can you show the link to that?

https://github.com/apache/cassandra-gocql-driver/blob/trunk/CONTRIBUTING.md

2. Yes, why not? I don't see a reason why we should merge it separately.

Maybe someone wants to try using testcontainers featute ASAP 😄

@smiklosovic
Copy link
Contributor Author

smiklosovic commented Sep 4, 2024

@ribaraka

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.

@ribaraka
Copy link

ribaraka commented Sep 5, 2024

Have you tried to bump it to 1.23 to see if it still works?

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.

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.

#1686 (comment)
I assume maintainers of the gocql were focused on supporting the last magor go version due to support of security, vulnerability updates and optimization.

From the Go Release Policy :

Each major Go release is supported until there are two newer major releases. For example, Go 1.5 was supported until the Go 1.7 release, and Go 1.6 was supported until the Go 1.8 release. We fix critical problems, including critical security problems, in supported releases as needed by issuing minor revisions (for example, Go 1.6.1, Go 1.6.2, and so on).

@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.

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.

OK, I will do it.

@ribaraka
Copy link

ribaraka commented Sep 5, 2024

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:

Run go vet
go: downloading gopkg.in/inf.v0 v0.9.1
go: downloading github.com/hailocab/go-hostpool v0.0.0-20160125115350-e80d13ce29ed
go: downloading github.com/golang/snappy v0.0.3
go: extracting gopkg.in/inf.v0 v0.9.1
go: extracting github.com/hailocab/go-hostpool v0.0.0-20160125115350-e80d13ce29ed
go: extracting github.com/golang/snappy v0.0.3
go: finding github.com/golang/snappy v0.0.3
go: finding github.com/hailocab/go-hostpool v0.0.0-20160125115350-e80d13ce29ed
go: finding gopkg.in/inf.v0 v0.9.1
# github.com/gocql/gocql [github.com/gocql/gocql.test]
Error: ./dial.go:104:18: tconn.HandshakeContext undefined (type *tls.Conn has no field or method HandshakeContext)
Error: ./marshal.go:1410:8: undefined: time.UnixMilli
Error: ./marshal.go:1420:8: undefined: time.UnixMilli
Error: Process completed with exit code 2.

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.

@smiklosovic
Copy link
Contributor Author

smiklosovic commented Sep 5, 2024

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.

@smiklosovic smiklosovic mentioned this pull request Sep 5, 2024
@absurdfarce
Copy link
Contributor

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.

@smiklosovic
Copy link
Contributor Author

sure we can wait if you think that would be better

@ribaraka
Copy link

@smiklosovic, I've added a fix for the flaky tests.

You can just create PRs against my PR and when I merge it into this PR it will run the pipeline again

I’ve submitted a PR for your review. Let me know if you have any feedback.

@smiklosovic
Copy link
Contributor Author

smiklosovic commented Sep 12, 2024

@ribaraka something is broken, I think you need to update go to 1.23 in the matrix

@ribaraka
Copy link

@smiklosovic done! re-push please

@ribaraka
Copy link

@smiklosovic I've added documentation on how to run tests locally c9e43bd.

The work on setting up Cassandra v5.0.0 is in progress.

@smiklosovic
Copy link
Contributor Author

ok great, thank you, just put all stuff to your branch and ping me once done

@ribaraka
Copy link

ribaraka commented Sep 19, 2024

I've consolidated all the work into the #1780 PR

@smiklosovic ^

@smiklosovic
Copy link
Contributor Author

@absurdfarce @ribaraka this might go in as we are after 1.7.0?

@smiklosovic
Copy link
Contributor Author

@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.

@joao-r-reis
Copy link
Contributor

Can we close this and focus on #1780 ? Or are the 2 PRs doing different things?

@ribaraka
Copy link

ribaraka commented Oct 6, 2024

@ribaraka can you please resolve the conflicts after we released 1.7.0?

done

@ribaraka
Copy link

ribaraka commented Oct 6, 2024

Can we close this and focus on #1780 ? Or are the 2 PRs doing different things?

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.

@michaelsembwever michaelsembwever changed the title CASSANDRA-19796 CASSANDRA-19796 – cassandra-gocql-driver CI Oct 10, 2024
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.

5 participants