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

add new google_cloudbuild_trigger resource #1357

Merged
merged 9 commits into from
May 7, 2018
Merged

add new google_cloudbuild_trigger resource #1357

merged 9 commits into from
May 7, 2018

Conversation

mathyourlife-fitbit
Copy link
Contributor

Resource is not fully featured as the build triggers are still in beta, but allows for resources as the one used in the acceptance test. All attributes marked as ForceNew as .Patch() calls have failed for me for when changing just something like the description.

resource "google_cloudbuild_trigger" "build_trigger" {
  project  = "${google_project_services.acceptance.project}"
  description = "acceptance test build trigger"
  trigger_template {
    branch_name = "master"
    project     = "${google_project_services.acceptance.project}"
    repo_name   = "some-repo"
  }
  build {
    images = ["gcr.io/$PROJECT_ID/$REPO_NAME:$COMMIT_SHA"]
    tags = ["team a", "service b"]
    step {
      name = "gcr.io/cloud-builders/gsutil"
      args = "cp gs://mybucket/remotefile.zip localfile.zip "
    }
    step {
      name = "gcr.io/cloud-builders/go"
      args = "build my_package"
    }
    step {
      name = "gcr.io/cloud-builders/docker"
      args = "build -t gcr.io/$PROJECT_ID/$REPO_NAME:$COMMIT_SHA -f Dockerfile ."
    }
  }
}

@mathyourlife-fitbit
Copy link
Contributor Author

Will need to add a website/docs/r/google_cloudbuild_trigger.html.markdown

@nat-henderson
Copy link
Contributor

Are you ready for review? Ping me when you are. :)

@kim0
Copy link

kim0 commented May 5, 2018

Patiently waiting for this to be ready :)

@mathyourlife-fitbit
Copy link
Contributor Author

mathyourlife-fitbit commented May 7, 2018

Sorry for the delay. @ndmckinley, should be ready for a review now.

Documentation has been added and the branch rebased against master. Travis shows an The command "make website-test" exited with 2. but test passes locally. Nav bar link and docs/providers/google/r/cloudbuild_trigger.html also render in local docker environment.

@nat-henderson nat-henderson self-requested a review May 7, 2018 18:10
@nat-henderson
Copy link
Contributor

Great. Working on that now. The make website-test command is a little bit broken, so we can ignore that for now. I am running the tests now.

Copy link
Contributor

@nat-henderson nat-henderson left a comment

Choose a reason for hiding this comment

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

Looks nearly perfect. Just two things that should be pulled out into their own functions for style consistency.

build.Tags = convertStringArr(v.([]interface{}))
}
stepCount := d.Get("build.0.step.#").(int)
if stepCount > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd convert this to use make unconditionally, and then append() rather than build.Steps[s] = step; I'm not going to insist on it. Either way, we have a pattern for these complex data structures, we like to have them in their own functions, parallel to the flatteners you already wrote (we call them expanders).

buildTrigger.Build = build
}

if d.Get("trigger_template.#").(int) > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Also its own function, please. :)

@nat-henderson
Copy link
Contributor

And the tests passed, FYI!

@mathyourlife-fitbit
Copy link
Contributor Author

Thanks, @ndmckinley. Added expanders to match flatteners and switched to make/cap for []*cloudbuild.BuildStep. Reran acceptance tests on my org, and passed again from this end.

Copy link
Contributor

@nat-henderson nat-henderson left a comment

Choose a reason for hiding this comment

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

Testing, then will merge.

@nat-henderson nat-henderson merged commit 4f7a6c8 into hashicorp:master May 7, 2018
@mathyourlife-fitbit
Copy link
Contributor Author

Thanks for the help, @ndmckinley 👍

@DSh3p4rd
Copy link

Any plans to support github? Any plans to support the include/exclude file filters?

@nat-henderson
Copy link
Contributor

@DSh3p4rd - is there an issue already open for that?

chrisst pushed a commit to chrisst/terraform-provider-google that referenced this pull request Nov 9, 2018
@ghost
Copy link

ghost commented Nov 17, 2018

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

@ghost ghost locked and limited conversation to collaborators Nov 17, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants