Skip to content

[no ticket][risk=low] update tables to latest #4

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

Merged
merged 8 commits into from
Dec 18, 2020

Conversation

jaycarlton
Copy link
Collaborator

@jaycarlton jaycarlton commented Dec 15, 2020

Bring all configs up-to-date, which is still slightly behind, but a good stopping point. We should be able to add a new column now.

@jaycarlton jaycarlton changed the title Jaycarlton/update tables2 [no ticket][risk=low] update tables to latest Dec 16, 2020
@jaycarlton jaycarlton requested a review from calbach December 16, 2020 15:10
@jaycarlton jaycarlton marked this pull request as ready for review December 16, 2020 15:10
project = var.project_id
region = var.region
zone = var.zone
project = "all-of-us-workbench-test"
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this for testing? Revert?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You need it for terraform import to work in 0.13. Otherwise it tells you it doesn't know how to find the provider. Hopefully it can be removed soon.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't follow, how does this get overridden when we use this module for an environment that is not all-of-us-workbench-test ? If it's not getting overridden, and the module still works properly for other projects, what is this parameter actually doing then?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point. I think the right way to do it is to specify the provider only at the highest level.


# TODO(jaycarlton) codegen this top-level variables as the union
# of all modules' variables files.
# List of objects whose values correspond to the google_monitoring_notification_channel
Copy link
Contributor

Choose a reason for hiding this comment

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

This stuff doesn't look relevant to the PR - remove? Minimally, should remove commented out code

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, thought I I needed it, but I guess not.

Copy link
Contributor

@calbach calbach left a comment

Choose a reason for hiding this comment

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

I'll approve so you aren't blocked tomorrow, but please address my comments

@jaycarlton jaycarlton merged commit 2f07ff5 into main Dec 18, 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.

2 participants