-
Notifications
You must be signed in to change notification settings - Fork 59
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
Bump scylla version to 5.4.6 and use fixed version for all of the nodes #187
Conversation
The CI does not work. It looks like there is a problem with the |
@sylwiaszunejko , in general I think it is bad idea to run tests on dynamic tags, better pick something static and update it time to time |
I agree after I saw the logs from failing container I agree we should be more thoughtful about changing these versions. |
@@ -8,3 +8,4 @@ client_encryption_options: | |||
keyfile: /etc/scylla/db.key | |||
truststore: /etc/scylla/ca.crt | |||
require_client_auth: true | |||
force_schema_commit_log: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to explicitly mention this param?
What about other params that are set by default in the Scylla.yaml?
Are we overriding all of them using this yaml?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without this the container fails to start. There is an issue about that when using 5.4.x we have to specify that option: scylladb/scylladb#16427 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sylwiaszunejko , can I ask you to add comment to the file so that we could link this option to the context
ba0bd31
to
6a19fca
Compare
CI works now, @avelanarius could you look at it? |
@@ -8,3 +8,4 @@ client_encryption_options: | |||
keyfile: /etc/scylla/db.key | |||
truststore: /etc/scylla/ca.crt | |||
require_client_auth: true | |||
force_schema_commit_log: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sylwiaszunejko , can I ask you to add comment to the file so that we could link this option to the context
@@ -33,7 +33,7 @@ services: | |||
timeout: 5s | |||
retries: 18 | |||
node_2: | |||
image: scylladb/scylla-nightly | |||
image: scylladb/scylla-nightly:6.0.0-rc2-0.20240602.cbf47319c1f7 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@roydahan , @sylwiaszunejko , WDYT does it work to have somethign like SCYLLA_TABLET_IMAGE
, instead of hardcoded one?
Only problem I see here is that driver-matrix test can use this either docker-compose.yaml
or integration.sh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure, when we have 6.0 we would not need to have different scylla version for tablet/non-tablet tests and then there will be no need for SCYLLA_TABLET_IMAGE
, we will just bump SCYLLA_IMAGE
to scylladb/scylla:6.0.0
and use it for all of the nodes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed
Previously scylla integration tests used scylla 5.2.9 and tablet tests used
scylladb/scylla-nightly
.This PR changes scylla version to
scylladb/scylla:5.4.6
for node used to integration test and to fixed version of scylla-nightly for tablet tests.