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

azurerm_[linux|windows]_virtual_machine - add Data Disks support #10466

Closed
wants to merge 21 commits into from

Conversation

jackofallops
Copy link
Member

@jackofallops jackofallops commented Feb 4, 2021

This PR creates an Opt-in beta for allowing the use of Data Disks in-line with the Linux and Windows Virtual Machine resources. Support for local disk creation and attachment of existing disks is included.

create disks are new disks created with the VM and can be partially managed (e.g. can be grown)
attach disks are exiting managed disks, which cannot be managed via the VM configuration.

By default, create data disks are deleted on VM deletion, which is controlled by the features block in the provider configuration. attach disks are always left intact by the VM.

resolves #6117
Fixes #8794

Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@jackofallops jackofallops marked this pull request as ready for review February 11, 2021 07:15
@jackofallops jackofallops added this to the v2.48.0 milestone Feb 11, 2021
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.

Taking a look through on the whole this looks pretty good - I've left some comments inline, mostly fairly minor around naming - but main issue here is around the context, where we're firing both the Update and Read contexts off in the Update, where the Read has a smaller timeout (and these are staggered) - instead we should pass through the Update context through the entire Update method, so that we're using that timeout for this entire process. Also worth noting there's no docs for this at this time?

azurerm/internal/features/user_flags.go Outdated Show resolved Hide resolved
azurerm/internal/provider/features.go Outdated Show resolved Hide resolved
azurerm/internal/provider/features.go Outdated Show resolved Hide resolved
azurerm/internal/provider/features_test.go Outdated Show resolved Hide resolved
azurerm/internal/provider/features_test.go Outdated Show resolved Hide resolved
azurerm/internal/services/compute/virtual_machine.go Outdated Show resolved Hide resolved
azurerm/internal/services/compute/virtual_machine.go Outdated Show resolved Hide resolved
azurerm/internal/services/compute/virtual_machine.go Outdated Show resolved Hide resolved
azurerm/internal/services/compute/virtual_machine.go Outdated Show resolved Hide resolved
@jackofallops jackofallops modified the milestones: v2.56.0, Blocked Apr 14, 2021
@jackofallops
Copy link
Member Author

Closing this for the time being due to technical limitations on implementation that are going to be resolved in future, but no set timeframe is currently available. Will re-open / start afresh as soon as possible.

@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 29, 2021
@jackofallops jackofallops deleted the f/vm-data-disk-beta branch September 10, 2021 09:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants