-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Deprecate createSampleWorkflows and provisionGmek fields. Add createSampleIntegrations #10478
Deprecate createSampleWorkflows and provisionGmek fields. Add createSampleIntegrations #10478
Conversation
Hello! I am a robot. Tests will require approval from a repository maintainer to run. @SarahFrench, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look. You can help make sure that review is quick by doing a self-review and by running impacted tests locally. |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
Breaking Change(s) DetectedThe following breaking change(s) were detected within your pull request.
If you believe this detection to be incorrect please raise the concern with your reviewer. |
@SarahFrench The change is expected. We do not have any usage for this resource and hence this change should be fine. |
Tests analyticsTotal tests: Click here to see the affected service packages
|
e1fc04d
to
2a967a6
Compare
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
Breaking Change(s) DetectedThe following breaking change(s) were detected within your pull request.
If you believe this detection to be incorrect please raise the concern with your reviewer. |
Tests analyticsTotal tests: Click here to see the affected service packages
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi there, thanks for your PR!
@SarahFrench The change is expected. We do not have any usage for this resource and hence this change should be fine.
Renaming a field is typically a breaking change, that's navigated via a deprecation process, because we don't know how many users may have a Terraform configuration that contains that field. I can see that this resource was only added last month but how can you be sure that this doesn't break a user's existing configuration?
2a967a6
to
5baae2c
Compare
5baae2c
to
6311288
Compare
Added deprecation message |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_integrations_client" "primary" {
create_sample_workflows = # value needed
}
|
Tests analyticsTotal tests: Click here to see the affected service packages
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_integrations_client" "primary" {
create_sample_workflows = # value needed
}
|
Tests analyticsTotal tests: Click here to see the affected service packages
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, thanks for adding the deprecation message! There's a 6.0.0 major release planned soon so the deprecated field can be removed then. Could you please open an issue about removing the field? That'll ensure the work is included in our planning.
Also, something I investigated was whether this PR should use the api_name
property, which we use to let a resource's fields have different names to API fields. I found that it's not currently usable because the deprecated field is present, so the existing solution with pre_create should be kept. However when the deprecated field is removed in future I think we should replace the use of the pre_create
with adding api_name: createSampleWorkflows
to the section describing createSampleIntegrations.
I'm currently not able to use this API in my GCP project, so could you please confirm via manual test or acceptance test that a resource can be provisioned using either the old or the new field? Thanks!
6311288
to
392cdd4
Compare
@SarahFrench Filed hashicorp/terraform-provider-google#17928 as request. I have also manually tested that resource could be provisioned with either fields. |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_integrations_client" "primary" {
create_sample_workflows = # value needed
}
|
Tests analyticsTotal tests: Click here to see the affected service packages
|
392cdd4
to
8160849
Compare
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: Click here to see the affected service packages
Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccIntegrationsClient_integrationsClientSampleWorkflowsExample |
|
8160849
to
33b3e1d
Compare
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: Click here to see the affected service packages
Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccIntegrationsClient_integrationsClientBasicExample |
|
33b3e1d
to
28710e2
Compare
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
28710e2
to
a85417b
Compare
Tests analyticsTotal tests: Click here to see the affected service packages
Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccIntegrationsClient_integrationsClientDeprecatedFieldsExample |
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: Click here to see the affected service packages
Action takenFound 9 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccIntegrationsAuthConfig_integrationsAuthConfigAdvanceExample|TestAccIntegrationsAuthConfig_integrationsAuthConfigAuthTokenExample|TestAccIntegrationsAuthConfig_integrationsAuthConfigClientCertificateOnlyExample|TestAccIntegrationsAuthConfig_integrationsAuthConfigJwtExample|TestAccIntegrationsAuthConfig_integrationsAuthConfigOauth2AuthorizationCodeExample|TestAccIntegrationsAuthConfig_integrationsAuthConfigOauth2ClientCredentialsExample|TestAccIntegrationsAuthConfig_integrationsAuthConfigOidcTokenExample|TestAccIntegrationsAuthConfig_integrationsAuthConfigServiceAccountExample|TestAccIntegrationsAuthConfig_integrationsAuthConfigUsernameAndPasswordExample |
|
Hi @SarahFrench |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved, but here's a summary of the PR in case anyone looks back at it:
create_sample_integrations
added as a renamed version ofcreate_sample_workflows
- The API isn't updated yet; the
pre_create
code will allow the new, renamed version of the field to be matched to the relevant API field name. - Use of
pre_create
is necessary instead ofapi_name
because of name collision between the deprecated and new versions of the field. When the deprecated field is removed use of api_name would be preferred, if its still necessary.
- The API isn't updated yet; the
- Tests confirm either field can be used currently
- provision_gmek is deprecated and not related to the create_sample_* fields above
Deprecate
createSampleWorkflows
andprovisionGmek
fields and addcreateSampleIntegrations
ingoogle_integrations_client
to keep it consistent with terminology.Release Note Template for Downstream PRs (will be copied)