adding terraform and tests #371
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
danieldeleo
requested changes
Jul 11, 2023
Comment on lines
76
to
81
| -var="project_id=${PROJECT_ID}" \ | ||
| -var="storage_project_id=${STORAGE_PROJECT_ID}" \ | ||
| -var="source_dataset_name=${SOURCE_DATASET_NAME}" \ | ||
| -var="target_dataset_name=${TARGET_DATASET_NAME}" \ | ||
| -var="crontab_format=${CRONTAB_FORMAT}" \ | ||
| -var="seconds_before_expiration=${SECONDS_BEFORE_EXPIRATION}" \ |
There was a problem hiding this comment.
Recommend using a .tfvars file and have the user modify this file with their own values
Comment on lines
102
to
107
| message = {"crontab_format":"0 1 * * *"} | ||
| timestamps = [] | ||
| for i in range(3): | ||
| timestamps.append(get_snapshot_timestamp) | ||
| time.sleep(1) | ||
| print(timestamps) |
There was a problem hiding this comment.
What's the purpose of this?
| # limitations under the License. | ||
|
|
||
| terraform { | ||
| backend "local" {} |
There was a problem hiding this comment.
Recommend using GCS remote backend
tools/cloud_functions/bq_table_snapshots/terraform/variables.tf
Outdated
Show resolved
Hide resolved
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
||
| terraform { |
There was a problem hiding this comment.
Run terraform fmt on all your terraform files
Co-authored-by: Daniel De Leo <danieldeleo@users.noreply.github.com>
Co-authored-by: Daniel De Leo <danieldeleo@users.noreply.github.com>
Co-authored-by: Daniel De Leo <danieldeleo@users.noreply.github.com>
Co-authored-by: Daniel De Leo <danieldeleo@users.noreply.github.com>
…_tables_names/test_bq_backup_fetch_tables_names.py Co-authored-by: Daniel De Leo <danieldeleo@users.noreply.github.com>
…e_snapshots/test_bq_backup_create_snapshots.py Co-authored-by: Daniel De Leo <danieldeleo@users.noreply.github.com>
Co-authored-by: Daniel De Leo <danieldeleo@users.noreply.github.com>
…e_snapshots/test_bq_backup_create_snapshots.py Co-authored-by: Daniel De Leo <danieldeleo@users.noreply.github.com>
Co-authored-by: Daniel De Leo <danieldeleo@users.noreply.github.com>
Co-authored-by: Daniel De Leo <danieldeleo@users.noreply.github.com>
…e_snapshots/test_bq_backup_create_snapshots.py Co-authored-by: Daniel De Leo <danieldeleo@users.noreply.github.com>
…_tables_names/test_bq_backup_fetch_tables_names.py Co-authored-by: Daniel De Leo <danieldeleo@users.noreply.github.com>
danieldeleo
requested changes
Jul 12, 2023
tools/cloud_functions/bq_table_snapshots/terraform/deployment_vars.tfvars
Outdated
Show resolved
Hide resolved
tools/cloud_functions/bq_table_snapshots/terraform/deployment_vars.tfvars
Outdated
Show resolved
Hide resolved
| @@ -37,12 +37,43 @@ The following environemnt variables must be set: | |||
| ## bq_backup_create_snapshots | |||
| The **bq_backup_create_snapshots** cloud function will submit a BigQuery job to create a snapshot for each table in scope. This cloud function will suffix the snapshot name with the snapshot datetime to guarantee a unique name. It will also calculate and set the expiration time of the snapshot based on seconds_before_expiration. Finally, it will determine the snapshot time based on crontab_format. | |||
| The following environemnt variables must be set: | |||
There was a problem hiding this comment.
Reword this and line 32 as well since it instructs the user to set an environment variable but the Terraform will actually do this.
The following could be an appropriate rewording:
"The Cloud Function uses the following environment variables to determine which BigQuery project to use for compute and which to use for storing your BigQuery snapshots:"
There was a problem hiding this comment.
Also lines 32 & 39 have typo: environemnt
Fix: environment
tools/cloud_functions/bq_table_snapshots/terraform/variables.tf
Outdated
Show resolved
Hide resolved
tools/cloud_functions/bq_table_snapshots/terraform/variables.tf
Outdated
Show resolved
Hide resolved
tools/cloud_functions/bq_table_snapshots/terraform/variables.tf
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Line 16 replace Scheduloer with Scheduler
There was a problem hiding this comment.
Line 35 has typo: ame
Fix: name
…vars.tfvars Co-authored-by: Daniel De Leo <danieldeleo@users.noreply.github.com>
…vars.tfvars Co-authored-by: Daniel De Leo <danieldeleo@users.noreply.github.com>
Co-authored-by: Daniel De Leo <danieldeleo@users.noreply.github.com>
Co-authored-by: Daniel De Leo <danieldeleo@users.noreply.github.com>
Co-authored-by: Daniel De Leo <danieldeleo@users.noreply.github.com>
Co-authored-by: Daniel De Leo <danieldeleo@users.noreply.github.com>
Co-authored-by: Daniel De Leo <danieldeleo@users.noreply.github.com>
Co-authored-by: Daniel De Leo <danieldeleo@users.noreply.github.com>
Co-authored-by: Daniel De Leo <danieldeleo@users.noreply.github.com>
Co-authored-by: Daniel De Leo <danieldeleo@users.noreply.github.com>
Co-authored-by: Daniel De Leo <danieldeleo@users.noreply.github.com>
Co-authored-by: Daniel De Leo <danieldeleo@users.noreply.github.com>
Co-authored-by: Daniel De Leo <danieldeleo@users.noreply.github.com>
danieldeleo
reviewed
Jul 12, 2023
| The following environemnt variables must be set: | ||
| * `BQ_DATA_PROJECT_ID `id of project used for BQ storage | ||
| The **bq_backup_create_snapshots** Cloud Function will submit a BigQuery job to create a snapshot for each table in scope. This Cloud Function will suffix the snapshot name with the snapshot datetime to guarantee a unique name. It will also calculate and set the expiration time of the snapshot based on seconds_before_expiration. Finally, it will determine the snapshot time based on crontab_format. | ||
| The following environment variables must be set: |
There was a problem hiding this comment.
Suggested change
| The following environment variables must be set: | |
| The Cloud Function uses the following environment variables to determine which BigQuery project to use for compute and which to use for storing your BigQuery snapshots: |
danieldeleo
reviewed
Jul 12, 2023
danieldeleo
reviewed
Jul 12, 2023
tools/cloud_functions/bq_table_snapshots/terraform/variables.tf
Outdated
Show resolved
Hide resolved
danieldeleo
approved these changes
Jul 12, 2023
Co-authored-by: Daniel De Leo <danieldeleo@users.noreply.github.com>
Co-authored-by: Daniel De Leo <danieldeleo@users.noreply.github.com>
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.