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

Initial module implementation #1

Merged
merged 32 commits into from
Mar 4, 2019
Merged

Conversation

Jberlinsky
Copy link
Contributor

@Jberlinsky Jberlinsky commented Dec 20, 2018

Notable design decisions that need addressing, but shouldn't prevent an initial implementation:

TODO

  • Docker image management needs to be updated to our new standards (see GKE & project-factory modules)

@morgante morgante self-requested a review December 27, 2018 20:18
@adrienthebo adrienthebo self-requested a review January 14, 2019 16:20
Copy link

@adrienthebo adrienthebo left a comment

Choose a reason for hiding this comment

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

Summary

Blockers:

  • Update terraform Docker image to build successfully
  • Convert IAM resources to be additive, not authoritative
  • Verify behavior around worker startup script, and possibly add test coverage
  • Amend test setup documentation

Standards conformance:

  • Quote boolean barewords as strings for v0.12

Review:

  • [x] Verify behavior around shared VPCs and firewall rules - or push shared VPC support to a later PR Deferred
  • [x] Consider merging firewall rules into a single rule

Suggestions:

  • Rename gce.tf to main.tf
  • Merge jenkins.tf and gce.tf/main.tf

jenkins.tf Outdated Show resolved Hide resolved
outputs.tf Outdated Show resolved Hide resolved
gce.tf Outdated Show resolved Hide resolved
gce.tf Outdated Show resolved Hide resolved
jenkins.tf Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
iam.tf Outdated
members = ["serviceAccount:${google_service_account.jenkins.email}"]
}

resource "google_project_iam_binding" "jenkins-network_admin" {

Choose a reason for hiding this comment

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

What would happen if a shared VPC would be used?

test/fixtures/shared/outputs.tf Show resolved Hide resolved
gce.tf Outdated Show resolved Hide resolved
@Jberlinsky Jberlinsky changed the title Initial module implementation [WIP] Initial module implementation Jan 16, 2019
Copy link

@adrienthebo adrienthebo left a comment

Choose a reason for hiding this comment

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

This is an old review that didn't get posted so these comments may be out of date. When I ran through the integration tests I had some issues building the packer image. If this PR is ready for more functional review we can dig into this next week.

test/fixtures/all_examples/build_image.sh Show resolved Hide resolved
test/fixtures/all_examples/build_image.sh Outdated Show resolved Hide resolved
@adrienthebo
Copy link

@Jberlinsky checking in - would you like me to do another review pass or would you like to get more work on it first?

@Jberlinsky
Copy link
Contributor Author

@adrienthebo Working on the current tranche of feedback now -- I'll ping you shortly for re-review :)

@Jberlinsky
Copy link
Contributor Author

@adrienthebo Might I suggest that we extract concerns related to shared VPCs to a separate PR, and proceed with reviewing this one as it stands?

Copy link

@adrienthebo adrienthebo left a comment

Choose a reason for hiding this comment

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

Summary

Previous review (#1 (review)) - fully addressed

Blockers

  • (trivial) We shouldn't attach the default compute service account when building the image; a patch has been added to fix this.

Suggested

  • (trivial) Drop the docker_build_kitchen_terraform dep from the docker_create target; an inline suggestion will fix this.

Defer

  • Shared VPC configuration
  • Control over Jenkins instance service accounts

Soft 👍 , let's get that fixup for Packer definition dealing with the default compute service account. Bonus points for the Makefile fixup, but it's a nonblocker. Ping me when the blocker has been resolved and let's get this merged!

Makefile Outdated Show resolved Hide resolved
examples/simple_example/packer/jenkins-agent.json Outdated Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
@Jberlinsky
Copy link
Contributor Author

@adrienthebo Ready for one more pass :)

@adrienthebo adrienthebo dismissed their stale review January 30, 2019 00:49

Updates made, needs an additional pass.

Copy link

@adrienthebo adrienthebo left a comment

Choose a reason for hiding this comment

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

LGTM! @morgante do you want to review this before we merge it?

@Jberlinsky Jberlinsky changed the title [WIP] Initial module implementation Initial module implementation Jan 30, 2019
@Jberlinsky
Copy link
Contributor Author

Pinging @morgante

Copy link
Contributor

@morgante morgante left a comment

Choose a reason for hiding this comment

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

Just some minor nits, I've also moved non-blockers out into issues.

firewall.tf Outdated Show resolved Hide resolved
firewall.tf Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
variables.tf Outdated Show resolved Hide resolved
variables.tf Outdated Show resolved Hide resolved
@Jberlinsky
Copy link
Contributor Author

@morgante I've address the issues mentioned here, would appreciate if you could take a final look so we can get this PR merged in!

@Jberlinsky Jberlinsky merged commit 0edd677 into master Mar 4, 2019
@aaron-lane aaron-lane deleted the feature/initial-implementation branch April 1, 2019 21:02
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