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

Reorganize custom DC Terraform script for readability #4786

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

kmoscoe
Copy link
Contributor

@kmoscoe kmoscoe commented Dec 11, 2024

  • Groups together variables for each service, and adds a heading for each group
  • Reorders services to follow workflow sequence (e.g. Redis comes at the end, if needed)
  • Moves API hostname and protocol to locals.tf so users can't change it

hqpho and others added 6 commits November 6, 2024 19:13
Highlights:
- Pins the version of `transformers` in nl_requirements.txt
- Adds support for a schema update mode in the data management
container, documented
[here](https://docs.datacommons.org/custom_dc/troubleshooting.html#schema-check-failed).
Schema check errors should also now have a direct link to the
troubleshooting page.
- Updates services container to exit as soon as any of the mixer, NL, or
website servers fails to start up
@kmoscoe kmoscoe requested a review from dwnoble December 11, 2024 21:14
Copy link
Contributor

@dwnoble dwnoble left a comment

Choose a reason for hiding this comment

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

I get this error when running terraform plan:

$ terraform plan
╷
│ Error: Reference to undeclared input variable
│ 
│   on locals.tf line 30, in locals:
│   30:   dc_api_root = "${var.dc_api_protocol}://${var.dc_api_hostname}"
│ 
│ An input variable with the name "dc_api_protocol" has not been declared. This variable can be declared with a variable "dc_api_protocol" {} block.
╵
╷
│ Error: Reference to undeclared input variable
│ 
│   on locals.tf line 30, in locals:
│   30:   dc_api_root = "${var.dc_api_protocol}://${var.dc_api_hostname}"
│ 
│ An input variable with the name "dc_api_hostname" has not been declared. This variable can be declared with a variable "dc_api_hostname" {} block.
╵

Looks like this is because dc_api_hostname was removed from the variables.tf. I think the options are:
(1) we keep the dc_api_hostname defined, but recommend users don't change it. Down the road, we may want to point the variable at "autopush.api.datacommons.org for our own internal testing instances.
(2) Add "api.datacommons.org" inline in locals.tf

I'm inclined to do (1) - wdyt @kmoscoe ?

@kmoscoe
Copy link
Contributor Author

kmoscoe commented Dec 12, 2024

I get this error when running terraform plan:

$ terraform plan
╷
│ Error: Reference to undeclared input variable
│ 
│   on locals.tf line 30, in locals:
│   30:   dc_api_root = "${var.dc_api_protocol}://${var.dc_api_hostname}"
│ 
│ An input variable with the name "dc_api_protocol" has not been declared. This variable can be declared with a variable "dc_api_protocol" {} block.
╵
╷
│ Error: Reference to undeclared input variable
│ 
│   on locals.tf line 30, in locals:
│   30:   dc_api_root = "${var.dc_api_protocol}://${var.dc_api_hostname}"
│ 
│ An input variable with the name "dc_api_hostname" has not been declared. This variable can be declared with a variable "dc_api_hostname" {} block.
╵

Looks like this is because dc_api_hostname was removed from the variables.tf. I think the options are: (1) we keep the dc_api_hostname defined, but recommend users don't change it. Down the road, we may want to point the variable at "autopush.api.datacommons.org for our own internal testing instances. (2) Add "api.datacommons.org" inline in locals.tf

I'm inclined to do (1) - wdyt @kmoscoe ?

Can we not just remove it altogether from locals.tf (I should have done that; sorry, my bad) or is that locals.tf file also used by the base Data Commons?

@dwnoble
Copy link
Contributor

dwnoble commented Dec 13, 2024

Thedc_api_root variable from locals.tf is used in the rest of the terraform deployment to point at the main data commons instance.

@kmoscoe
Copy link
Contributor Author

kmoscoe commented Dec 16, 2024

Thedc_api_root variable from locals.tf is used in the rest of the terraform deployment to point at the main data commons instance.

OK, in that case, I'd prefer to do option #1.

@dwnoble
Copy link
Contributor

dwnoble commented Dec 16, 2024

Sounds good to me

@kmoscoe
Copy link
Contributor Author

kmoscoe commented Dec 16, 2024 via email

@kmoscoe
Copy link
Contributor Author

kmoscoe commented Dec 17, 2024

Sorry, I meant option 2!

OK done!

@kmoscoe kmoscoe requested a review from dwnoble December 17, 2024 00:07
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