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

Requiring existing resources to be imported / support for timeouts #1746

Closed
wants to merge 154 commits into from
Closed

Requiring existing resources to be imported / support for timeouts #1746

wants to merge 154 commits into from

Conversation

tombuildsstuff
Copy link
Contributor

@tombuildsstuff tombuildsstuff commented Aug 9, 2018 •
edited
Loading

This PR fixes a couple of cross-resource bugs in the Provider:

  • Firstly this works around the fact most of the Azure API's are Upserts by requiring that resources are present in the state before creating them
  • This adds support for Timeouts, which fixes Resource Timeouts for creation / deletion #171

This is being done in one hit so it's consistent, rather than being inconsistent across resources.

Resource Support

  • App Service
  • Application Gateway
  • Application Insights
  • Application Security Group
  • Automation
  • Auto Scale
  • Availability Set
  • AzureAD
  • CDN
  • Container Group
  • Container Registry
  • Container Service
  • CosmosDB
  • Data Lake
  • DNS
  • EventGrid
  • EventHub
  • Express Route
  • Function App
  • Image
  • IoTHub
  • Key Vault
  • Kubernetes
  • Local Network Gateway
  • Log Analytics
  • Managed Disks
  • Management Locks
  • Monitor
  • MySQL
  • Networks
  • Notification Hub
  • Packet Capture
  • Policy
  • Postgres
  • Public IP
  • Recovery Services
  • Redis
  • Relay
  • Roles
  • Routes
  • Scheduler Job
  • Search
  • Service Fabric
  • ServiceBus
  • Snapshot
  • SQL
  • Subnets
  • Traffic Manager
  • User Assigned Identity
  • Virtual Network Gateways

virtual resources are slightly different:

  • Load Balancer
  • Logic Apps

requires more extensive testing:

  • Storage
  • Template Deployments
  • Virtual Machines
  • Virtual Machine Disk Attachments
  • Virtual Machine Extentions
  • VM Scale Sets

needs to be done last / in this order:

  • Networks
  • Resource Groups

other

  • remove the default timeout in config.go
  • removes a bunch of crash points in ACS
  • Data Lake Store File ID dependent on Data Lake Store File: updating the ID #1856
  • azurerm_dns_cname_record - removing the removed records field
  • Dependent on Storage: Import Support #1816 merged
  • Upgrade to AutoRest v11
  • Update to include latest PR's once merged:
    • API Management
    • Automation DSC Configuration
    • Automation DSC Node Configuration
    • Automation Module
    • Cognitive Services Account
    • Databricks Workspace
    • DevSpace Controller
    • DevTest Lab (Data Source & Resource)
    • DevTest Linux Virtual Machine
    • DevTest Policy
    • DevTest Virtual Network
    • DevTest Windows Virtual Machine
    • Firewall
    • Firewall Network Rule Collection
    • Log Analytics Workspace Linked Service
    • Management Groups
    • Monitor Activity Log Alert
    • Monitor Metric Alert
    • Monitor Log Profile
    • MySQL Virtual Network Rule
    • Network Interface <-> Application Gateway Backend Address Pool Association
    • Network Interface <-> Load Balancer Backend Address Pool Association
    • Network Interface <-> Load Balancer NAT Rule Association
    • PostgreSQL Virtual Network Rule
    • Recovery Services Protection Policy VM
    • Recovery Services Protected VM
    • Security Center Contact
    • Security Center Subscription Pricing
    • Security Center Workspace
    • Shared Image (Data Source & Resource)
    • Shared Image Gallery (Data Source & Resource)
    • Shared Image Version (Data Source & Resource)
    • Subnet <-> Network Security Group Association
    • Subnet <-> Route Table Association

@tombuildsstuff tombuildsstuff added this to the 1.13.0 milestone Aug 9, 2018
This was referenced Aug 10, 2018
Closed
Closed
@tombuildsstuff tombuildsstuff force-pushed the existence-and-timeouts branch 2 times, most recently from a322204 to 85c99df Compare August 14, 2018 17:34
@tombuildsstuff tombuildsstuff modified the milestones: 1.13.0, 1.14.0 Aug 15, 2018
@tombuildsstuff tombuildsstuff mentioned this pull request Aug 16, 2018
Closed
3 tasks
@metacpp
Copy link
Contributor

metacpp commented Aug 16, 2018 •
edited
Loading

Hey @tombuildsstuff

Considering that this PR contains so many files (~100), which will have big chance (see below conflicts) to conflict with other merged PRs in the near future. Will it be better to split such kind of mega PR into smaller ones and iterate fast? Just my 2 cents.

metacpp
metacpp reviewed Aug 17, 2018
View reviewed changes
azurerm/resource_arm_servicebus_topic_authorization_rule.go Outdated
resp, err := client.GetAuthorizationRule(ctx, resourceGroup, namespaceName, topicName, name)
if err != nil {
if !utils.ResponseWasNotFound(resp.Response) {
return fmt.Errorf("Error checking for the existence of Service Bus Topic Rule %q (Resource Group %q / Namespace %q): %+v", name, resourceGroup, namespaceName, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

  		return fmt.Errorf(" [](start = 0, length = 23)

Move this error message as template function in errors package?

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 varies wildly by resource, so this doesn't make sense to refactor out

@tombuildsstuff
Copy link
Contributor Author

@metacpp

Considering that this PR contains so many files (~100), which will have big chance (see below conflicts) to conflict with other merged PRs in the near future. Will it be better to split such kind of mega PR into smaller ones and iterate fast?

From the description:

This is being done in one hit so it's consistent, rather than being inconsistent across resources.

I'm rebasing this PR at least once a day - but unfortunately this needs to be applied to all resources at once since it's a behavioural change. Whilst we could potentially merge this in with a feature flag to disable it, we'd have to modify every file to remove that feature flag at some point anyway, so I don't see that as a beneficial option in this case

We don't often have cross-cutting PR's for this reason, but behavioural changes (such as this one, and also #1728) unfortunately sometimes warrant larger PR's so the UX is consistent.

@tombuildsstuff tombuildsstuff force-pushed the existence-and-timeouts branch 7 times, most recently from 039d656 to ed58e57 Compare August 22, 2018 15:51
@ghost ghost added the size/XXL label Aug 23, 2018
@ghost ghost added the size/XXL label Aug 23, 2018
This was referenced Aug 23, 2018
Merged
Closed
@ghost ghost added the size/XXL label Sep 14, 2018
This was referenced Sep 19, 2018
Closed
Closed
@tombuildsstuff tombuildsstuff modified the milestones: 1.16.0, 1.17.0 Sep 21, 2018
@tombuildsstuff tombuildsstuff modified the milestones: 1.17.0, Soon Oct 5, 2018
This was referenced Oct 15, 2018
Closed
Closed
@tombuildsstuff tombuildsstuff modified the milestones: Soon, Blocked Oct 25, 2018
@tombuildsstuff tombuildsstuff mentioned this pull request Oct 25, 2018
Closed
@tombuildsstuff tombuildsstuff mentioned this pull request Nov 6, 2018
Merged
@pixelicous

This comment has been minimized.

Sign in to view
@tombuildsstuff tombuildsstuff mentioned this pull request Nov 8, 2018
Closed
@nmkleiberg

This comment has been minimized.

Sign in to view
@tombuildsstuff tombuildsstuff mentioned this pull request Dec 13, 2018
Merged
@tombuildsstuff
Copy link
Contributor Author

Closing this in favour of #2511

@ghost
Copy link

ghost commented Mar 5, 2019

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 and limited conversation to collaborators Mar 5, 2019
@tombuildsstuff tombuildsstuff deleted the existence-and-timeouts branch October 2, 2019 05:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@metacpp metacpp metacpp left review comments

@mbfrahry mbfrahry mbfrahry approved these changes

Assignees
No one assigned
Labels
enhancement hashibot/ignore size/XXL technical-debt
Projects
None yet
Milestone
Blocked
Development

Successfully merging this pull request may close these issues.

Resource Timeouts for creation / deletion
6 participants
@tombuildsstuff @metacpp @pixelicous @nmkleiberg @mbfrahry @katbyte