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

chore: improve scylla container health check #580

Merged
merged 1 commit into from
Aug 1, 2024
Merged

Conversation

cisse21
Copy link
Member

@cisse21 cisse21 commented Aug 1, 2024

Description

  1. Add an option to create a keyspace while creating resource
  2. Improve the scylla health check mechanism

Linear Ticket

Fixes PIPE-1374

Security

  • The code changed/added as part of this pull request won't create any security issues with how the software is being used.

@cisse21 cisse21 changed the title chore: test scylla chore: improve scylla container health check Aug 1, 2024
@cisse21 cisse21 marked this pull request as ready for review August 1, 2024 09:54
@@ -2,6 +2,8 @@ module github.com/rudderlabs/rudder-go-kit

go 1.22.5

replace github.com/gocql/gocql => github.com/scylladb/gocql v1.14.2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are we pinning this version?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought this was a good practice to pin the version as we are doing the same in rudder-server for the replace block

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also have this error replacement module without version must be directory path (rooted or starting with . or ..) if I remove the version

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get it:

	module declares its path as: github.com/gocql/gocql
	        but was required as: github.com/scylladb/gocql

So we can't do much. The sensible way is to replace with latest but then a go mod tidy would replace latest with the version. And that's what you got now so it's cool.

replace github.com/gocql/gocql => github.com/scylladb/gocql latest

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically they forked without changing the path:

https://github.com/scylladb/gocql/blob/master/go.mod

module github.com/gocql/gocql

require (
	github.com/bitly/go-hostpool v0.0.0-20171023180738-a3a6125de932 // indirect
	github.com/bmizerany/assert v0.0.0-20160611221934-b7ed37b82869 // indirect
	github.com/golang/snappy v0.0.3
	github.com/google/go-cmp v0.4.0
	github.com/hailocab/go-hostpool v0.0.0-20160125115350-e80d13ce29ed
	github.com/kr/pretty v0.1.0 // indirect
	github.com/stretchr/testify v1.3.0 // indirect
	golang.org/x/net v0.0.0-20220526153639-5463443f8c37
	gopkg.in/inf.v0 v0.9.1
	sigs.k8s.io/yaml v1.3.0
)

retract (
    v1.8.0  // tag from kiwicom/gocql added by mistake to scylladb/gocql
    v1.8.1  // tag from kiwicom/gocql added by mistake to scylladb/gocql
    v1.9.0  // tag from kiwicom/gocql added by mistake to scylladb/gocql
    v1.10.0 // tag from kiwicom/gocql added by mistake to scylladb/gocql
)

go 1.13

Copy link
Collaborator

@fracasula fracasula left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question as Leo about the gocql version. The rest looks good 👍

@cisse21 cisse21 merged commit 7428dce into main Aug 1, 2024
10 checks passed
@cisse21 cisse21 deleted the chore.addTestScylla branch August 1, 2024 11:06
@psrikanth88 psrikanth88 mentioned this pull request Aug 1, 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.

3 participants