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

Update universal/scalardl module to use schema loader image for Cassandra #185

Merged
merged 2 commits into from
Aug 18, 2020

Conversation

ymorimo
Copy link
Contributor

@ymorimo ymorimo commented Aug 14, 2020

https://scalar-labs.atlassian.net/browse/DLT-6879

This PR changes universal/scalardl module to use scalardl-schama-loader-cassandra for the initial schema creation. The image is needed to be transferred to one of the scalardl hosts in the same way as the scalar-ledger image.

The cqlsh and create_schema.cql script in scalar-ledger image is no longer used. (They are going to be removed in the next version of scalar-ledger.)

@ymorimo ymorimo requested review from feeblefakie and tei-k August 14, 2020 03:46
@ymorimo ymorimo self-assigned this Aug 14, 2020
@@ -13,6 +16,7 @@ resource "null_resource" "scalardl_image" {

triggers = {
scalar_tag = var.scalardl_image_tag
triggers = join(",", var.triggers)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why add triggers here ? Do we need to rebuild the image locally due to changes in bastion or Cassandra?

@@ -27,7 +31,6 @@ resource "null_resource" "scalardl_image_push" {

triggers = {
docker_image = null_resource.scalardl_image[0].id
triggers = join(",", var.triggers)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it related to this PR?
Maybe this is necessary, because if the bastion is replaced, we need to upload again to bastion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I intended to modify the "triggers" tree but I'm bit confused with it. I'd like to discuss this later.
1

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! I understand that this is an improvement on the triggers. 👍

@@ -148,7 +191,7 @@ resource "null_resource" "scalardl_schema" {

provisioner "remote-exec" {
inline = [
"docker run -e CASSANDRA_REPLICATION_FACTOR=${var.replication_factor} --rm ${local.scalar_image} dockerize -template create_schema.cql.tmpl:create_schema.cql -wait tcp://${local.scalar_cassandra_host}:9042 -timeout 30s cqlsh --cqlversion=3.4.4 ${local.scalar_cassandra_host} -u '${var.cassandra_username}' -p '${var.cassandra_password}' -f create_schema.cql"
"docker run -e CASSANDRA_HOST='${local.scalar_cassandra_host}' -e CASSANDRA_USERNAME='${var.cassandra_username}' -e CASSANDRA_PASSWORD='${var.cassandra_password}' -e CASSANDRA_REPLICATION_FACTOR=${var.replication_factor} --rm ${local.schema_loader_cassandra_image}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why use single quotes? for escaping?
I'm a little concerned when the password contains single quotes.

Copy link
Contributor Author

@ymorimo ymorimo Aug 16, 2020

Choose a reason for hiding this comment

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

It seems that single quotations are used because they are surrounded by double quotations. The problem is the same as before. I will check if it fails if the username or password contains any single quotation, and report it as a bug later.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!
It's an existing problem, and I think it's fine to consider it separately.

Copy link
Collaborator

@feeblefakie feeblefakie left a comment

Choose a reason for hiding this comment

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

LGTM !

Copy link
Contributor

@tei-k tei-k left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

@feeblefakie feeblefakie merged commit 2a9c1f9 into master Aug 18, 2020
@feeblefakie feeblefakie deleted the schema-loader-cassandra branch August 18, 2020 00:31
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