Skip to content

Hello world #32

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

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

Hello world #32

wants to merge 18 commits into from

Conversation

kollerdaniel
Copy link
Contributor

@kollerdaniel kollerdaniel commented Nov 18, 2021

This change is Reviewable

@csabakassai
Copy link
Member

put all the tf files into a terraform folder, to keep them separated from the rest of files

@csabakassai
Copy link
Member

csabakassai commented Nov 19, 2021

do a 'terraform fmt' to properly format the tf files

@@ -0,0 +1,92 @@
provider "google" {
credentials = file(var.credentials_file)
Copy link
Member

Choose a reason for hiding this comment

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

do not specify the credentials here, use the application default.
https://cloud.google.com/sdk/gcloud/reference/auth/application-default/login
It is is much safer this way

@@ -0,0 +1,92 @@
provider "google" {
credentials = file(var.credentials_file)
project = var.project
Copy link
Member

Choose a reason for hiding this comment

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

it is better if you specify these parameters on the resources explicitly than here.
This can lead to confusing errors, if you work in an environment with multiple projects and locations

}

resource "google_bigquery_dataset" "dataset" {
dataset_id = "example_dataset"
Copy link
Member

Choose a reason for hiding this comment

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

call this jarvis_sample_01


resource "google_bigquery_dataset" "dataset" {
dataset_id = "example_dataset"
friendly_name = "test"
Copy link
Member

Choose a reason for hiding this comment

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

this is an optional parameter so either fill it with meaningful value or remove it.
This is just a temporary dataset, so this is not really needed

resource "google_bigquery_dataset" "dataset" {
dataset_id = "example_dataset"
friendly_name = "test"
description = "This is a test description"
Copy link
Member

Choose a reason for hiding this comment

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

this is an optional parameter so either fill it with meaningful value or remove it.
This is just a temporary dataset, so this is not really needed

schema = <<EOF
[
{
"name": "ID",
Copy link
Member

Choose a reason for hiding this comment

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

be consistent either use uppercase everywhere or lowercase everywhere for the columnnames

dataset_id = google_bigquery_dataset.dataset.dataset_id
table_id = "car_output"

time_partitioning {
Copy link
Member

Choose a reason for hiding this comment

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

you do not need this here, this is just an ephemeral small table

time_partitioning {
type = "DAY"
}
labels = {
Copy link
Member

Choose a reason for hiding this comment

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

this is an optional parameter so either fill it with meaningful value or remove it.
This is just a temporary dataset, so this is not really needed

{
"name": "ID",
"type": "INTEGER",
"mode": "REQUIRED",
Copy link
Member

Choose a reason for hiding this comment

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

what is the transformation you are planning to execute, why is the schema of the table the same?

@@ -0,0 +1,2 @@
project = "koller-dani-sandbox"
credentials_file = "C:/Users/kollerdani/Desktop/koller-dani-sandbox-10e37a32eb02.json"
Copy link
Member

Choose a reason for hiding this comment

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

in general, you should never put anything into the code repository that only works in your local environment.

@csabakassai
Copy link
Member

add tf specific gitignore

@@ -0,0 +1,72 @@
provider "google" {
region = "europe-west1"
Copy link
Member

Choose a reason for hiding this comment

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

do not define region and zone here

@@ -0,0 +1 @@
project = "koller-dani-sandbox"
Copy link
Member

Choose a reason for hiding this comment

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

do not add this to the repo, just keep it locally

.gitignore Outdated

*.tfstate
*.tfstate.*
*.tfvars
Copy link
Member

Choose a reason for hiding this comment

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

"zh:a853f1a1e9f5ac261fd9a02d869ce7da16917fdd16464dabf2200e2ccd04fba9",
"zh:aba631b9db47cc34857c25459475f17d939a46022f886e396a8254588f720c58",
"zh:ac461358984ba480b4fef1d2be3538d9714aa8814a6f420e1e954227054a14a4",
"zh:ade64689485fc28b6aa0c36861c6c9c66b5d7a8e7282739ecf97c7791b973312",
Copy link
Member

Choose a reason for hiding this comment

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

remove the exe below from git. This is downloaded automatically during the tf init, so not needed in version control

Copy link
Contributor

@nbali nbali left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r1.
Reviewable status: 0 of 13 files reviewed, 19 unresolved discussions (waiting on @csabakassai, @kollerdaniel, and @nbali)

a discussion (no related file):
asdasd


@nbali nbali self-requested a review September 7, 2022 13:20
Copy link
Contributor

@nbali nbali left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 7 files at r2, 1 of 8 files at r6, 3 of 3 files at r7, 4 of 4 files at r12, 1 of 1 files at r13, 1 of 2 files at r15, 2 of 2 files at r16, all commit messages.
Reviewable status: all files reviewed, 18 unresolved discussions (waiting on @kollerdaniel)

Copy link
Contributor

@nbali nbali left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed (commit messages unreviewed), 18 unresolved discussions (waiting on @kollerdaniel)

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