Skip to content

Conversation

ymorimo
Copy link
Contributor

@ymorimo ymorimo commented Nov 12, 2020

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

Add database, database_contact_points and database_contact_port variables to Terraform modules.

To-Dos

The latest scalar-ledger image and scalardl-schema-loader image don't support DynamoDB yet. Their versions should be updated to work with DynamoDB.

@ymorimo ymorimo self-assigned this Nov 12, 2020
@ymorimo ymorimo requested review from feeblefakie and tei-k November 12, 2020 10:51
@ymorimo ymorimo marked this pull request as ready for review November 12, 2020 10:51
@ymorimo ymorimo marked this pull request as draft November 12, 2020 11:08
@ymorimo ymorimo marked this pull request as ready for review November 13, 2020 16:44
@ymorimo ymorimo force-pushed the add-database-option-to-universal-scalardl branch from 8a37236 to 57dc7cc Compare November 13, 2020 19:35
The DynamoDB resion should be specified in `database_contact_points` instead
since scalardb uses contact_points as a region.
@ymorimo ymorimo force-pushed the add-database-option-to-universal-scalardl branch from 57dc7cc to fcf2512 Compare November 13, 2020 19:37
@ymorimo ymorimo marked this pull request as draft November 15, 2020 23:55
@ymorimo ymorimo marked this pull request as ready for review November 16, 2020 00:20
@feeblefakie feeblefakie requested a review from yito88 November 19, 2020 01:26
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!

@feeblefakie
Copy link
Collaborator

@tei-k Please take a look!

"%s %s",
"docker run --rm ${var.schema_loader_image} -h ${var.database_contact_points} -u ${var.database_username} -p ${var.database_password}",
var.database == "cassandra" ? "--cassandra -P ${var.database_contact_port} -n NetworkTopologyStrategy -R ${var.cassandra_replication_factor}" :
var.database == "dynamo" ? "--dynamo --region ${var.database_contact_points}" :
Copy link
Contributor

@tei-k tei-k Nov 24, 2020

Choose a reason for hiding this comment

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

@ymorimo

I'm not sure how it works with --region ${var.database_contact_points}. 💦

var.database_contact_points is dynamodb.${region}.amazonaws.com ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. When DynamoDB is used, the region name should be specified in the contact_points variable.
https://github.com/scalar-labs/scalardb/blob/master/docs/getting-started-with-dynamodb.md#configure-scalar-db
This is also added in the database_contact_points variable's description. https://github.com/scalar-labs/scalar-terraform/pull/237/files#diff-c29567323bde55c7871f3e1b1fd9242c4f268d7d82129d0eb5fce499a78319d4R18

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.

Overall looking good. Left one qustion.

Copy link
Member

@yito88 yito88 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 96cb192 into dynamodb Nov 24, 2020
@feeblefakie feeblefakie deleted the add-database-option-to-universal-scalardl branch November 24, 2020 12:54
@ymorimo ymorimo mentioned this pull request Nov 30, 2020
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.

4 participants