Skip to content

Conversation

@zachberger
Copy link
Contributor

This Cloud Function detects newly created VM Instances that have disks attached
with a Google-managed encryption key attached and then immediately deletes
any such VM instances.

@morgante morgante self-assigned this May 31, 2019
@aaron-lane aaron-lane added the enhancement New feature or request label Jun 3, 2019
Copy link
Contributor

@aaron-lane aaron-lane left a comment

Choose a reason for hiding this comment

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

I verified the Terraform configuration is functional, but could the example be extended with a noncompliant VM to prove the functionality without requiring the end users to add additional resources?

@zachberger
Copy link
Contributor Author

could the example be extended with a noncompliant VM to prove the functionality without requiring the end users to add additional resources?

This sounds frustrating. Typically when I grab an example or tool I want it to work with as little configuration as possible. If I do this the end user has to cleanup the TF and remove this resource and any other dependencies.

@zachberger
Copy link
Contributor Author

@aaron-lane I don't have the ability to assign this back to you

@morgante
Copy link
Contributor

This sounds frustrating. Typically when I grab an example or tool I want it to work with as little configuration as possible. If I do this the end user has to cleanup the TF and remove this resource and any other dependencies.

Agreed.

@aaron-lane
Copy link
Contributor

@zachberger Typically, examples should be as self-contained as possible. If this example has practical applications outside of demonstrating usage of the module then we should add it as a submodule so that it can be properly supported.

@zachberger
Copy link
Contributor Author

This is exhausting. Ive been sent from repo to repo being told to contribute in different ways. At this point I don't even want to contribute this anymore.

@zachberger zachberger closed this Jun 11, 2019
@morgante
Copy link
Contributor

I'm reopening this because I concur with @zachberger that having an entire VM instance be part of the config just for a simple example is extraneous and unnecessary. As far as I am concerned, this PR is fine to merge as-is.

@morgante morgante reopened this Jun 11, 2019
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 one tiny nit.

}
}

func RecieveMessage(ctx context.Context, msg *pubsub.Message) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit. spelling (ReceiveMessage)

@morgante morgante requested a review from aaron-lane June 11, 2019 21:39
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.

One tiny nit, but I am fine with merging as-is.

@morgante morgante merged commit d11cae8 into terraform-google-modules:master Jun 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants