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.

Sign up for GitHub

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

Jump to bottom

Update azure-sdk-for-go to v46.1.0 #8411

Merged
merged 16 commits into from
Sep 21, 2020
Merged

Update azure-sdk-for-go to v46.1.0 #8411

merged 16 commits into from
Sep 21, 2020

Conversation

ArcturusZhang
Copy link
Contributor

No description provided.

@ghost ghost added the size/XXL label Sep 10, 2020
WodansSon
WodansSon approved these changes Sep 10, 2020
View reviewed changes
Copy link
Collaborator

@WodansSon WodansSon left a comment

Choose a reason for hiding this comment

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

@ArcturusZhang Thanks for the PR! LGTM! 🚀

tombuildsstuff
tombuildsstuff suggested changes Sep 11, 2020
View reviewed changes
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

hi @ArcturusZhang

Thanks for this PR - I've taken a look through and it appears there's a couple of changes necessary here to not send a misleading value here?

Thanks!

azurerm/internal/services/apimanagement/api_management_user_resource.go Outdated
@@ -145,7 +145,7 @@ func resourceArmApiManagementUserCreateUpdate(d *schema.ResourceData, meta inter
properties.UserCreateParameterProperties.State = apimanagement.UserState(state)
}

if _, err := client.CreateOrUpdate(ctx, resourceGroup, serviceName, userId, properties, ""); err != nil {
if _, err := client.CreateOrUpdate(ctx, resourceGroup, serviceName, userId, properties, nil, ""); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

what's this nil value here? as per the last PR this is expecting where this resource is being created/updated - so sending the Old/New Portal is misleading - presumably we should be sending terraform here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This new parameter is notify - I will try to pass terraform to appType in other two methods and see if it works

Copy link
Contributor

Choose a reason for hiding this comment

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

@ArcturusZhang can we set this to true/false (matching the default value used in the other resources) here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem. On my way. BTW do we need to revert those changes on the apimanagement identity?

azurerm/internal/services/apimanagement/api_management_subscription_resource.go Outdated Show resolved Hide resolved
@imod imod mentioned this pull request Sep 14, 2020
Closed
@tombuildsstuff tombuildsstuff added this to the v2.29.0 milestone Sep 18, 2020
tombuildsstuff
tombuildsstuff approved these changes Sep 18, 2020
View reviewed changes
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

hi @ArcturusZhang

Taking a look through here, if we can fix up the one comment (to pass the same notify value as the other resources) - and the tests look good then this otherwise seems fine 👍

Thanks!

azurerm/internal/services/apimanagement/api_management_user_resource.go Outdated
@@ -145,7 +145,7 @@ func resourceArmApiManagementUserCreateUpdate(d *schema.ResourceData, meta inter
properties.UserCreateParameterProperties.State = apimanagement.UserState(state)
}

if _, err := client.CreateOrUpdate(ctx, resourceGroup, serviceName, userId, properties, ""); err != nil {
if _, err := client.CreateOrUpdate(ctx, resourceGroup, serviceName, userId, properties, nil, ""); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

@ArcturusZhang can we set this to true/false (matching the default value used in the other resources) here?

@ArcturusZhang
Copy link
Contributor Author

Maybe we should also remove the None from identity.0.type candidate list and also change type from Optional to Required to align the document.
I initially worked around this in a different approach since I assume removing an option / changing an attribute from optional to required might be considered as a breaking change.

@notchairmk notchairmk mentioned this pull request Sep 18, 2020
Merged
@tombuildsstuff tombuildsstuff merged commit c283ea4 into hashicorp:master Sep 21, 2020
tombuildsstuff added a commit that referenced this pull request Sep 21, 2020
@tombuildsstuff
updating to include #8411
19671f8
@ArcturusZhang ArcturusZhang deleted the update-v46.1.0-sdk branch September 22, 2020 00:12
@ghost
Copy link

ghost commented Sep 24, 2020

This has been released in version 2.29.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 2.29.0"
}
# ... other configuration ...

@flo-02-mu flo-02-mu mentioned this pull request Sep 29, 2020
Closed
@ghost
Copy link

ghost commented Oct 22, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

@ghost ghost locked as resolved and limited conversation to collaborators Oct 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@WodansSon WodansSon WodansSon approved these changes

@tombuildsstuff tombuildsstuff tombuildsstuff approved these changes

Assignees

@ArcturusZhang ArcturusZhang

Labels
dependencies size/XXL
Projects
None yet
Milestone
v2.29.0
Development

Successfully merging this pull request may close these issues.
3 participants
@ArcturusZhang @tombuildsstuff @WodansSon