Hi!
Now any changes of cloud build trigger create new one, this PR makes triggers modifiable in place.
I referred to [bigquery dataset](9f0b792f09/google/resource_bigquery_dataset.go (L353)). If theare are better examples, I'd like to know them.
Match the bigtable_instance resource to the API structure, deprecating
the inlined config parameters. This is a complicated deprecation,
because we don't want to ForceNew when updating to use the new
undeprecated fields, but we want to ForceNew if they change.
This is achieved using a CustomizeDiff.
Refactor `google_sql_database_instance` to use Expand functions in Create. Update will start using them when we make the resource authoritative, this change does not modify the resource functionality.
Update (Regional) Instance Group Managers to deprecate the beta fields
(rolling_update_policy, versions, and auto_healing_policy). Also, rename
RESTART to REPLACE for Instance Group Manager's update_strategy, to be a
bit clearer. This required a DiffSuppress to make sure RESTART and
REPLACE are considered equivalent. In 2.0.0, we should remove RESTART
and drop the DiffSuppressFunc. Sadly, we have no way to do a deprecation
message for RESTART. We'll have to put it in the CHANGELOG. I've already
removed it from the website.
Flattening disks involves taking the relative link of each disk. This
only works if you start from a self_link, but we were starting from
anything that resolveImage returns, which includes several
not-self_links.
This PR adds a test that shows the failure, and then fixes it by
expanding the output back out to a self_link before getting the relative
link from it.
Deprecate the app_engine sub-block of google_project, and create a
google_app_engine_application resource instead. Also, add some tests for
its behaviour, as well as some documentation for it.
Note that this is largely an implementation of the ideas discussed in
#2118, except we're not using CustomizeDiff to reject deletions without
our special flag set, because CustomizeDiff apparently doesn't run on
Delete. Who knew? This leaves us rejecting the deletion at apply time,
which is less than ideal, but the only other option I see is to silently
not delete the resource, and that's... not ideal, either.
This also stops the app_engine sub-block on google_project from forcing
new when it's removed, and sets it to computed, so users can safely move
from using the sub-block to using the resource without state surgery or
deleting their entire project. This does mean it's impossible to delete
an App Engine application from a sub-block now, but seeing as that was
the same situation before, and we just papered over it by making the
project recreate itself in that situation, and people Were Not Fans of
that, I'm considering that an acceptable casualty.
Fix Dana's comments, one of which exposed a bug: we weren't actually
fixing the diff. Because resolveImage can return multiple formats, and
only returns a self_link if a self_link is passed in, it was diffing
more often than not against a self_link supplied in the config. We just
had a bug in our CustomizeDiff function that concealed this.
I added the resolvedImageSelfLink helper function to turn the output
from resolveImage into a self_link, which fixes the problem. It also has
the knock-on effect of fixing #2067 at the same time, which is nice. I
decided to add another function instead of just modifying resolveImage
to always return a self_link because I don't want to deal with the
backwards compatibility problems of changing how we're storing a bunch
of things in state this close to 1.19.0. And honestly, we should
probably just be storing the self_link always, _anyways_.
We don't need quite so many `GetOk`s since the client library will ignore any fields that are set to the zero value for that type. I left a few that involved error-handling or things that had to be set before other things, but this at least should make the code a bit nicer to look at.
Tests are passing except the ones that were already failing in CI.
When reading or deleting the google_project_service resource, first
retrieve the project. If it's not found, or if it's pending deletion,
remove the service, as it's functionally disabled.
Also, add a test to confirm the behaviour.
This should resolve#1292.
I'm working on Stackdriver monitoring in another branch, which required updating `google.golang.org/genproto`, which required updating `google.golang.org/grpc`, which required updating `github.com/golang/protobuf`, and so on. This PR updates all of the Google-provided deps to their latest versions. In addition, there is:
- A change in config.go to reflect an updated type name
- Five files changed by `make fmt`
Tested with `make build`, `make test`, and `make testacc`.
I also did a bit of cleanup while I was here and noticed things that I thought could be improved in the files (wording changes, removing tests that aren't quite necessary, etc.) Take a look and make sure I didn't actually remove anything important!
So, every time this test fails we are leaving behind 10 CPUs. This is a big cause of our quota problem because we are at 70!! right now. We still need to fix the test but let's also reduce this count to 1 so we leave 1/10th of the zombie instances.
Changing from a new to legacy health check also did not work so I have no clue what the issue is here. Observing the instances, they are shown as being stuck in "Instance is being verified" state for up to days.
We had a failure in the last CI run while waiting for this resource to be created, so let's increase the timeout.
```
------- Stdout: -------
=== RUN TestAccDataSourceRegionInstanceGroup
--- FAIL: TestAccDataSourceRegionInstanceGroup (631.53s)
testing.go:518: Step 0 error: Error applying: 1 error(s) occurred:
* google_compute_region_instance_group_manager.foo: 1 error(s) occurred:
* google_compute_region_instance_group_manager.foo: timeout while waiting for state to become 'created' (last state: 'creating', timeout: 5m0s)
testing.go:579: Error destroying resource! WARNING: Dangling resources
may exist. The full state and error is shown below.
Error: Error refreshing: 1 error(s) occurred:
* google_compute_region_instance_group_manager.foo: 1 error(s) occurred:
* google_compute_region_instance_group_manager.foo: google_compute_region_instance_group_manager.foo: timeout while waiting for state to become 'created' (last state: 'creating', timeout: 5m0s)
State:
FAIL
```
The hadoop test passes for me locally but fails in CI, so this should give some extra debugging information (my user acct doesn't have permissions to read the dataproc logs from the failing test).
WIP for now because I want to make sure this method of reading the logs works.
TestAccComputeDisk_timeout is expected to fail 27.77% of the time because acctest.RandString can start with a number, making an invalid `name`. Let's fix that :)
The ID that is created for a nodepool is 3 parts in the form of
<location>/<cluster>/<nodepool>. When we use the newer 4 part import
format the ID becomes that string. Then when checking the existence of
the nodepool the name is incorrectly parsed out of the ID.
When using the 4 part import form we need to set the correct ID so that
terraform can parse the name from the ID.
Closes: #1846
* Adding resource_attached_disk
This is a resource which will allow joining a arbitrary compute disk
to a compute instance. This will enable dynamic numbers of disks to
be associated by using counts.
* undelete-update recently soft-deleted custom roles
* remove my TODO statements
* check values on soft-delete-recreate for custom role tests
* final fixes to make sure delete works; return read() when updating to 'create'
* check for non-404 errors for custom role get
* add warnings to custom roles docs
Fixes#1494.
* Add import support for `google_logging_organization_sink`, `google_logging_folder_sink`, `google_logging_billing_account_sink`.
Using `StateFunc` over `DiffSuppressFunc` should only affect tests; for some reason `TestAccLoggingFolderSink_folderAcceptsFullFolderPath` expected a `folder` value of `folders/{{id}}` vs expecting `{{id}}` when only `DiffSuppressFunc` was used, when in real use `DiffSuppressFunc` should be sufficient.
When using instance templates, if you use one of our image shorthands at
the moment, you'll get a perma-diff. This is because the config gets
resolved to another format before it is set in state, and so we need to
set that other format in state.
Unfortunately, resolving images requires network access, so we have to
do this with CustomizeDiff. CustomizeDiff was having trouble (I think?
More on this below) on setting a field as not ForceNew once the field
was already set, so I moved the decision for whether a field was
ForceNew or not into CustomizeDiff. I also resolved the old and new
images, and if they were the same, cleared the diff.
Unfortunately, you can't actually clear a field on a sub-block right
now. You have to clear top-level fields only. So this will currently
throw an error. I opened hashicorp/terraform#18795 to fix that. Once
that's merged, and we vendor it here, this patch fixes the problem.
If hashicorp/terraform#18795 doesn't get merged, the next best
workaround is to keep track of _all_ the fields under `disk` with a diff
in our CustomizeDiff, check whether they've all changed or not, and if
they've all changed, clear the changes on `disk`, which I _think_ will
resolve the issue. That's just a massive pain, unfortunately.
* fix service account key data source name
* switch id to name
* update docs
* doc format
* fixes for validation and tests
* last fixes for service account key data source
Previously, we were only setting source images for the disks in instance
templates if they weren't set in the config. Instead, let's just always
set them. This fixes our test failures about self_links not matching
names. Also, relative links are safer than just image names, as
Terraform wouldn't notice if another project's image of the same name
got used instead of your project's.
Also fix the setting of tags and tag fingerprints to always set, even if
to the empty value, to fix the tests.
* test empty authorized masters' cidr_blocks
When the `cidr_block` isn't simply blank but contains an empty list as in
```
master_authorized_networks_config {
cidr_blocks = []
}
```
a panic occurs looking something like
```
goroutine 26 [running]:
github.com/terraform-providers/terraform-provider-google/google.expandMasterAuthorizedNetworksConfig(0x15a4f80, 0xc4202586e0, 0x21)
/tmp/GOPATH/src/github.com/terraform-providers/terraform-provider-google/google/resource_container_cluster.go:1355 +0x4f2
github.com/terraform-providers/terraform-provider-google/google.resourceContainerClusterCreate(0xc420146a80, 0x16b1800, 0xc4200b8000, 0x0, 0x0)
/tmp/GOPATH/src/github.com/terraform-providers/terraform-provider-google/google/resource_container_cluster.go:520 +0x2848
github.com/terraform-providers/terraform-provider-google/vendor/github.com/hashicorp/terraform/helper/schema.(*Resource).Apply(0xc420495490, 0xc420341310, 0xc4202582c0, 0x16b1800, 0xc4200b8000, 0x1, 0xc42024eae0, 0xc4201e3650)
/tmp/GOPATH/src/github.com/terraform-providers/terraform-provider-google/vendor/github.com/hashicorp/terraform/helper/schema/resource.go:227 +0x364
github.com/terraform-providers/terraform-provider-google/vendor/github.com/hashicorp/terraform/helper/schema.(*Provider).Apply(0xc4204c6700, 0xc4203412c0, 0xc420341310, 0xc4202582c0, 0x14ee1441a000, 0x0, 0x18)
/tmp/GOPATH/src/github.com/terraform-providers/terraform-provider-google/vendor/github.com/hashicorp/terraform/helper/schema/provider.go:283 +0xa4
github.com/terraform-providers/terraform-provider-google/vendor/github.com/hashicorp/terraform/plugin.(*ResourceProviderServer).Apply(0xc4202d7c40, 0xc42035de80, 0xc42025c160, 0x0, 0x0)
/tmp/GOPATH/src/github.com/terraform-providers/terraform-provider-google/vendor/github.com/hashicorp/terraform/plugin/resource_provider.go:527 +0x57
reflect.Value.call(0xc4203feae0, 0xc42000e038, 0x13, 0x19e88a8, 0x4, 0xc42015ff20, 0x3, 0x3, 0xc420047ee8, 0xc4204c6798, ...)
/usr/local/go/src/reflect/value.go:434 +0x905
reflect.Value.Call(0xc4203feae0, 0xc42000e038, 0x13, 0xc420047f20, 0x3, 0x3, 0xc400000001, 0x0, 0x0)
/usr/local/go/src/reflect/value.go:302 +0xa4
net/rpc.(*service).call(0xc420418600, 0xc42007c140, 0xc42001e798, 0xc4200c4000, 0xc4202d6c40, 0x1557f80, 0xc42035de80, 0x16, 0x1557fc0, 0xc42025c160, ...)
/usr/local/go/src/net/rpc/server.go:381 +0x142
created by net/rpc.(*Server).ServeCodec
/usr/local/go/src/net/rpc/server.go:475 +0x36b
```
which we trigger by altering the first step to contain the HCL notation
for an empty list instead of simply an empty string.
In order to accomplish this, the tests had to be modified to accept an
emptyValue string as well which contains the content of the
`emptyValue` string when the cidrBlocks array is empty. This maintains
the old behavior of the original tests when `emptyValue` is an empty
string, while also facilating differing behavior for the new testcase by
setting `emptyValue` to whichever string we want to test instead. I
don't think this is very clean, but I guess it's pragmatic enough.
I'll hear if this is a thorn in the side to someone 😏.
* avoid panic on cidr_block type assertion
This is basically the fix. Since the value can be nil, we want to ensure
we handle a failure during the assertion since we know that asserting
`nil` conforms to `map[string]interface{}` will cause a run-time panic.
* flatten to config on empty list for cidr_blocks
since an empty list for cidrBlocks constitutes valid input, one should
return a map containing an empty list for the cidr_blocks field instead
of a nil value.
The nil value is only appropriate when the input Config is also nil.
* Update google_container_cluster master_auth username and password without recreation of cluster, using setMasterAuth method
* Add tests to update and disable master_auth password and username values
Fixes#1702.
@chrisst I'm putting you as a reviewer, but no rush. Feel free to ask as many questions as you have! Also feel free to offer suggestions 😃 (or just say it's perfect as-is, that works too)
We had a bad type coercion in the code to update container cluster
resource labels. This fixes the coercion.
We didn't notice this because there was no test exercising the update
code path. I've added the test, which reproduced the panic before this
PR was applied and passes successfully now that the coercion is fixed.
Images now have a licenses field, which lets users specify licenses to
use on an image. But official images already have licenses on them, adn
so Terraform is reading those as a diff. To preserve backwards
compatiblity and avoid a breaking change that would require all
`google_compute_image` users to update their configs, I've set the field
to computed. This means that if no licenses are set in the config, there
can be licenses in the config without prompting a diff. Obviously, this
isn't ideal, as it means you can't ever remove all the licenses from an
image, but I think the benefits here outweigh the drawbacks.
In testing an upcoming `google_compute_region_disk` resource, I had to make these changes. Checking them in separately so that when the magician runs, these changes will already be a part of TF.
Currently, the rolling-update API expects a PUT (regionInstanceGroupManager.update())
instead of a PATCH (regionInstanceGroupManager.patch()) call, so immediate rolling updates
(as specified with `update_strategy = "ROLLING_UPDATE"``) are never triggered:
```
[DEBUG]: ---[ REQUEST ]---------------------------------------
[DEBUG]: POST /compute/beta/projects/mycompany-myapp-staging/regions/europe-west3/instanceGroupManagers/myapp-server/setInstanceTemplate?alt=json HTTP/1.1
....
[DEBUG]: ---[ RESPONSE ]--------------------------------------
...
[DEBUG]: "warnings": [
[DEBUG]: {
[DEBUG]: "code": "FIELD_VALUE_OVERRIDEN",
[DEBUG]: "message": "Update policy type was set to OPPORTUNISTIC. Please use regionInstanceGroupManager.update() to preserve the policy."
[DEBUG]: }
```
refs:
https://cloud.google.com/compute/docs/reference/rest/beta/instanceGroupManagers/patchhttps://cloud.google.com/compute/docs/reference/rest/beta/instanceGroupManagers/updateFix#1506
This was done as its own resource as suggested in slack, since we don't have the option of making all fields Computed in google_compute_instance. There's precedent in the aws provider for this sort of thing (see ami_copy, ami_from_instance).
When I started working on this I assumed I could do it in the compute_instance resource and so I went ahead and reordered the schema to make it easier to work with in the future. Now it's not quite relevant, but I left it in as its own commit that can be looked at separately from the other changes.
Fixes#1582.
