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

Included terraform-nomad-postgres #21

Merged
merged 8 commits into from
Sep 2, 2020

Conversation

claesgill
Copy link
Contributor

@claesgill claesgill commented Aug 31, 2020

Closes #2

UPDATED:

  • Implemented terraform-nomad-postgres module instead of local postgres (used in both examples).
  • Removed postgres.hcl.
  • Added variables used in main.tf

OLD:

  • Implemented terraform-nomad-postgres module instead of local postgres.
  • Removed postgres.hcl.
  • Added variables used in main.tf and outputs to be available in example/datastack/main.tf

Since Redash now is dependent on the terraform-nomad-postgres-module, the module is referred to in the root main.tf. The though behind this is that you shouldn't need to create a postgres-module every time you want to create a redash-module.

This is a design choice that is up for descussion and the reason why this PR is a draft.

…ded variables used in main.tf and outputs to be available in example/datastack/main.tf
Copy link
Contributor

@fredrikhgrelland fredrikhgrelland left a comment

Choose a reason for hiding this comment

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

I do not think we should do this.

Any prerequisites should be explicit and importable into this module.
Generally we want all modules to be self contained AND composable. Adding a dependancy on a different terraform module will require us to support all changes to that module rather than keep it up to the consumers.
I also envision a future postgres module that can handle multiple databases. So, for dev/test purposes you will spinn up one postgres module and use that for redash/hive_metastore ++

@claesgill
Copy link
Contributor Author

I do not think we should do this.

Any prerequisites should be explicit and importable into this module.
Generally we want all modules to be self contained AND composable. Adding a dependancy on a different terraform module will require us to support all changes to that module rather than keep it up to the consumers.
I also envision a future postgres module that can handle multiple databases. So, for dev/test purposes you will spinn up one postgres module and use that for redash/hive_metastore ++

I think you have very good points and I agree with most of them. Because my understanding of a module is that it should be a "one-click service", where you can import the module without the knowlege of how it works. With that in mind I think the minimal import a user should need to get Redash up and running is:

module "redash" {
    source = "github.com/fredrikhgrelland/terraform-nomad-redash/?ref=<version>"
}

I agree that the modules should be self contained and composable. So could it be a solution to re-implement the postgres.hcl that was used before with the ability to override it with another postgres?

At the end I'm not sure of what is right or wrong, and I have no problem implementing your suggestion. My point is that, at least for me, it doesn't make sense that you would need to manually create a postgres to get redash to work.

@pdmthorsrud
Copy link
Contributor

I just wanna say I lean towards what Fredrik is saying. I talked briefly with Claes yesterday, and I echoed your statement, Fredrik. What gives me pause is that Redash would never work alone with that implementation, and why is that OK? Do we need to stop thinking of modules as services, and rather imagine them as components that could also be a service out of the box, but might need other components to constitute a full service?

@fredrikhgrelland
Copy link
Contributor

The main point in this is to keep persistent storage separate. If you implement postgres as a part of the service, we need to keep that state through updates blue/green deployments etc. This kind of state keeping is very important and the code is very specialised, best outsourced to a separate component.
It should however be noted that we should implement mandatory variables for postgres so that it is ever clear that this must be added as a side car to this module.

@claesgill
Copy link
Contributor Author

The main point in this is to keep persistent storage separate. If you implement postgres as a part of the service, we need to keep that state through updates blue/green deployments etc. This kind of state keeping is very important and the code is very specialised, best outsourced to a separate component.
It should however be noted that we should implement mandatory variables for postgres so that it is ever clear that this must be added as a side car to this module.

Thanks for a good explanation! That makes a valid point, and I think what you say make sense.

I'll fix it in the morning.

@claesgill claesgill marked this pull request as ready for review September 2, 2020 06:31
@fredrikhgrelland fredrikhgrelland self-requested a review September 2, 2020 06:54
Copy link
Contributor

@fredrikhgrelland fredrikhgrelland left a comment

Choose a reason for hiding this comment

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

Looks good.

Do you guys think we should split the main.tf in the examples folder into separate files?
I think it would be easier to update and to read?

either: keep redash module in main.tf and rest in dependencies.tf

or: keep redash module in main.tf and the rest split into minio.tf, postgres.tf etc.

@pdmthorsrud
Copy link
Contributor

Looks good.

Do you guys think we should split the main.tf in the examples folder into separate files?
I think it would be easier to update and to read?

either: keep redash module in main.tf and rest in dependencies.tf

or: keep redash module in main.tf and the rest split into minio.tf, postgres.tf etc.

I like option 2. Modules might have many inputs in the future, makes it easy and quick to figure out what's going on.

@claesgill
Copy link
Contributor Author

claesgill commented Sep 2, 2020

Looks good.

Do you guys think we should split the main.tf in the examples folder into separate files?
I think it would be easier to update and to read?

either: keep redash module in main.tf and rest in dependencies.tf

or: keep redash module in main.tf and the rest split into minio.tf, postgres.tf etc.

I also think option 2 would be great for readability. Quick question so that I understand this correctly. We will only need to create a module named redash in main.tf without importing the minio.tf, presto.tf etc since these are concatenated into main.tf by terraform?

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@pdmthorsrud
Copy link
Contributor

I also think option 2 would be great for readability. Quick question so that I understand this correctly. We will only need to create a module named redash in main.tf without importing the minio.tf, presto.tf etc since these are concatenated into main.tf by terraform?

Sorry, I didn't see this. Yes, that's correct!

@claesgill claesgill force-pushed the issue_2_include_postgres_module branch from 267867d to b4b21c9 Compare September 2, 2020 12:40

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@claesgill claesgill merged commit ad536ca into master Sep 2, 2020
@claesgill claesgill deleted the issue_2_include_postgres_module branch September 2, 2020 13:26
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.

Use: https://github.com/fredrikhgrelland/terraform-nomad-postgres/releases/tag/0.0.1
4 participants