adding KMS key encryption to disks#569
Conversation
|
Will add tests once #568 is merged. |
|
Looks like this failed in concourse: https://terraform.ci.cloud-graphite.com/builds/9663. I'm a bit confused why puppet tried to do anything with it, given that it's |
f2d7bce to
66e21c9
Compare
|
@danawillow it appears that MM never correctly handled 'beta' versions nested below a certain depth. I added a commit to strip out properties that are at the wrong version. |
b327967 to
d26bd76
Compare
|
Is that generated PR expected? It looks like it put the new field in the GA provider so I'm going to hold off on reviewing now assuming it wasn't expected. |
|
I guess the PR is kind of expected. I'm adding new beta fields with new custom code and new tests that depend on the beta fields. So I'm not sure how we have dealt with testing beta fields in the past, or more importantly how we deal with them now. But at least it's not generating chef and puppet code like it was before! |
|
Got it! We'll add protectors around the beta stuff- I'm working on that right now. It should be doable in the custom code parts with an |
danawillow
left a comment
There was a problem hiding this comment.
Looks good. I'd prefer this wait until my changes get in so you can exclude the test from the GA provider, but I'm willing to be convinced otherwise.
| resource "google_project_services" "apis" { | ||
| project = "${google_project.project.project_id}" | ||
| services = [ | ||
| "oslogin.googleapis.com", |
There was a problem hiding this comment.
nit: spaces vs tabs in this list
| // The raw key won't be returned, so we need to use the original. | ||
| transformed["rawKey"] = d.Get("disk_encryption_key.0.raw_key") | ||
| transformed["sha256"] = original["sha256"] | ||
| // The response for crypto keys often includes the version of the key which needs to be removed |
There was a problem hiding this comment.
You should add version checks in ruby here so that these don't appear in the GA version:
<% unless version.nil? || version == 'ga' %>
(you can do this now, without my changes)
|
I would agree except that d26bd76 solves a long standing MM bug If it's just 1 test that will be failing, I'm all for merging it in. |
|
It wouldn't be reverted though, it would start getting generated, right? Anyway yeah that's fine, it's just one test and we'll probably have it fixed by the end of tomorrow (if not sooner) anyway. |
b84e14b to
d3532dc
Compare
|
If it's not a big deal, I'd rather we do version comparisons against ga instead of against beta. If we were to add an alpha version, this feature would be a part of it. |
|
I am (still) a robot that works on MagicModules PRs! I just wanted to let you know that your changes (as of commit 2ff85db) have been included in your existing downstream PRs. |
1da4856 to
105ee40
Compare
Also adding to Image and Snapshot even though those resources aren't generated yet.
105ee40 to
d4bc1b4
Compare
|
I am a robot that works on MagicModules PRs! I built this PR into one or more PRs on other repositories, and when those are closed, this PR will also be merged and closed. |
Tracked submodules are build/puppet/_bundle build/puppet/auth build/puppet/bigquery build/puppet/compute build/puppet/sql build/puppet/storage build/puppet/spanner build/puppet/container build/puppet/dns build/puppet/pubsub build/puppet/resourcemanager build/chef/_bundle build/chef/auth build/chef/compute build/chef/sql build/chef/storage build/chef/spanner build/chef/container build/chef/dns build/chef/iam build/terraform-beta build/terraform build/ansible build/inspec.
Also adding to Image and Snapshot even though those resources aren't generated
yet.
[all]
[terraform]
adding KMS key encryption to disks
[puppet]
[puppet-bigquery]
[puppet-compute]
[puppet-container]
[puppet-dns]
[puppet-logging]
[puppet-pubsub]
[puppet-resourcemanager]
[puppet-sql]
[puppet-storage]
[chef]
[chef-compute]
[chef-container]
[chef-dns]
[chef-logging]
[chef-spanner]
[chef-sql]
[chef-storage]
[ansible]