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.

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

Fix ignore_changes behavior for AGW #8761

Merged
merged 7 commits into from
Oct 19, 2020
Merged

Fix ignore_changes behavior for AGW #8761

merged 7 commits into from
Oct 19, 2020

Conversation

kvendingoldo
Copy link
Contributor

No description provided.

@ztbrown
Copy link

ztbrown commented Oct 6, 2020

this looks good, but I would like to see a test. That's what was keeping me from PRing.

@kvendingoldo
Copy link
Contributor Author

@ztbrown Will try to do it today or tomorrow

@kvendingoldo
Copy link
Contributor Author

@ztbrown Was you thinking about unit tests OR manual testing of this change?

@ztbrown
Copy link

ztbrown commented Oct 6, 2020

unit / acceptance was what I was thinking

@ghost ghost added size/L and removed size/XS labels Oct 7, 2020
@kvendingoldo
Copy link
Contributor Author

@ztbrown I created a draft of unit test that should work for us. For now, I need to find a way how to change AGW certificate via Go code, after that it could be seen as a whole change.

}

return nil

Copy link

Choose a reason for hiding this comment

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

remove this newline

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why?

Copy link

Choose a reason for hiding this comment

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

I believe this is what is upsetting the linter.

@ztbrown
Copy link

ztbrown commented Oct 8, 2020

Nice work! Did you see that you're failing the linter?

image

Need to remove the newline on 4082.

@kvendingoldo
Copy link
Contributor Author

@ztbrown I've seen and will fix it a little bit later. For now, I'm not able to fix my test, have the following issue:

    TestAccAzureRMApplicationGateway_manualSslCertificateChangeIgnoreChanges: testing.go:684: Step 0 error: Check failed: Check 3/4 error: Bad: updating AGW: network.ApplicationGatewaysClient#CreateOrUpdate: Failure sending request: StatusCode=400 -- Original Error: Code="InvalidResourceReference" Message="Resource /subscriptions/XXX/resourceGroups/acctestRG-201008145214600822/providers/Microsoft.Network/applicationGateways/acctesttag/sslCertificates/acctestcertificate1 referenced by resource /subscriptions/XXX/resourceGroups/acctestRG-201008145214600822/providers/Microsoft.Network/applicationGateways/acctesttag/httpListeners/acctest-vnet-201008145214600822-httplstn was not found. Please make sure that the referenced resource exists, and that both resources are in the same region." Details=[]

Do you have any idea why agw.SslCertificates = &newSslCertificates isn't working fine?

@ghost ghost added dependencies size/XXL and removed size/L labels Oct 8, 2020
@ghost ghost added size/L and removed size/XXL labels Oct 8, 2020
@kvendingoldo
Copy link
Contributor Author

Screen Shot 2020-10-09 at 3 21 44 AM

@kvendingoldo
Copy link
Contributor Author

@ztbrown probably I finished with tests. For now I need to find a way of fixing linter issues and after that it could be reviewed and merged

@ztbrown
Copy link

ztbrown commented Oct 9, 2020

👍 Looking great!

@kvendingoldo

This comment has been minimized.

@kvendingoldo

This comment has been minimized.

Copy link
Member

@jackofallops jackofallops left a comment

Choose a reason for hiding this comment

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

Thanks @kvendingoldo - This LGTM 👍
Tests Passing:
image

@jackofallops jackofallops merged commit 5830e88 into hashicorp:master Oct 19, 2020
@jackofallops jackofallops added this to the v2.33.0 milestone Oct 19, 2020
jackofallops added a commit that referenced this pull request Oct 19, 2020
@ghost
Copy link

ghost commented Oct 22, 2020

This has been released in version 2.33.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.33.0"
}
# ... other configuration ...

@ghost
Copy link

ghost commented Nov 18, 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 👉 hashibot-feedback@hashicorp.com. Thanks!

@ghost ghost locked as resolved and limited conversation to collaborators Nov 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

application gateway resource not honoring lifecycle ignore_changes for ssl_certificates during apply
3 participants