-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
new resource "azurerm_maintenance_assignment" #6713
Conversation
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 @njuCZ
Thanks for this PR for the new resource and the continued work on this service.
A few of us have looked over this and we feel that it should probably be split into 2 discrete resources, one for vm
and another for dedicated_host
. This allows us to provide a better user experience, by being able to validate the Resource ID we’re expecting (either a Virtual Machine/Dedicated Host ID) - rather than falling back to the error messages from ARM, which aren’t clear when using an incorrect ID) - in addition to being able to parse it easier internally.
Aside from the overall design, the only comment is on the testing approach for "Readiness" and looking at replacing time.Sleep
with a more reliable mechanism.
{ | ||
// It may take a few minutes after starting a VM for it to become available to assign to a configuration | ||
// for newly created machine, wait several minutes | ||
PreConfig: func() { time.Sleep(5 * time.Minute) }, |
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.
Is there a state we can check / poll for that we can use for a more stable test? Using time.Sleep
is an approach we try to avoid, and are looking at removing the handful of other occurrences in the provider.
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.
For now I don't know, I will have a ask to the service team. This comment message comes from the rest api response when assigning a configuration to a newly created machine
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.
I got the reply from service team, they say there is a backlog item for it, but currently this is no flag to check
@jackofallops thanks for your opinion, I will split the resource. Before making these changes, I want to confirm that if maintenance assignment supports assign to other resource, we should add a new resource accordingly ? Beucase for now the design could cover all potential resources and the validation will tell users current support target resource
|
dda9caf
to
19f8b52
Compare
19f8b52
to
2522b35
Compare
@jackofallop I have splitted into two resources, could you please have a look again? |
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 @njuCZ
Thanks for splitting this out, it is certainly a lot clearer now. A few comments / questions below.
I think the question to the service team needs answering and addressing before we can merge these new resources though. If there's a delay between a VM creation and the ability to assign a maintenance configuration then this could cause apply failures for users that create these resources as a single configuration (given the nature of this resource, this feels a very likely use-case). We should have the assignments resource check it can apply (and possibly poll, if not ready) rather than having the tests sleep for ~5min, do you agree?
azurerm/internal/services/maintenance/maintenance_assignment_dedicated_host_resource.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/maintenance/maintenance_assignment_dedicated_host_resource.go
Show resolved
Hide resolved
azurerm/internal/services/maintenance/maintenance_assignment_dedicated_host_resource.go
Show resolved
Hide resolved
azurerm/internal/services/maintenance/maintenance_assignment_virtual_machine_resource.go
Show resolved
Hide resolved
azurerm/internal/services/maintenance/maintenance_assignment_virtual_machine_resource.go
Show resolved
Hide resolved
website/docs/r/maintenance_assignment_virtual_machine.html.markdown
Outdated
Show resolved
Hide resolved
website/docs/r/maintenance_assignment_virtual_machine.html.markdown
Outdated
Show resolved
Hide resolved
website/docs/r/maintenance_assignment_dedicated_host.html.markdown
Outdated
Show resolved
Hide resolved
website/docs/r/maintenance_assignment_dedicated_host.html.markdown
Outdated
Show resolved
Hide resolved
website/docs/r/maintenance_assignment_dedicated_host.html.markdown
Outdated
Show resolved
Hide resolved
@jackofallops thanks for your suggestions, I have added the retry logic and removed the sleep logic. Could you pelase have a look again ? |
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! |
fix #6037
virtual machine
anddedicated host
.location
andResourceID
is missed in the body, and fields are case insensitive