-
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_stack_hci_extension
#26929
Conversation
…m into stackhci_extension
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 @teowa,
Thanks for this PR - I've taken a look through and left some comments inline. If we can fix those up, this should be good to go 👍
existing, err := client.Get(ctx, *id) | ||
if err != nil { | ||
if response.WasNotFound(existing.HttpResponse) { | ||
return fmt.Errorf("%s was not found", id) |
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.
return fmt.Errorf("%s was not found", id) | |
return metadata.MarkAsGone(*id) |
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.
fixed
if response.WasNotFound(existing.HttpResponse) { | ||
return fmt.Errorf("%s was not found", id) | ||
} | ||
return fmt.Errorf("reading %s: %+v", id, err) |
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.
return fmt.Errorf("reading %s: %+v", id, err) | |
return fmt.Errorf("retrieving %s: %+v", *id, err) |
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.
changed
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.
seems not changed?
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.
sorry for missing this one, updated
|
||
model := resp.Model | ||
if model == nil || model.Properties == nil || model.Properties.ExtensionParameters == nil { | ||
return fmt.Errorf("retrieving %s: `model` was nil", id) |
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.
return fmt.Errorf("retrieving %s: `model` was nil", id) | |
return fmt.Errorf("retrieving %s: `model` was nil", *id) |
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.
fixed
model.Properties.ExtensionParameters.TypeHandlerVersion = pointer.To(config.TypeHandlerVersion) | ||
} | ||
|
||
if err := client.CreateThenPoll(ctx, *id, *model); err != nil { |
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.
update should use UpdateThenPoll
or CreateOrUpdateThenPoll
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.
fixed
publisher = "Microsoft.EnterpriseCloud.Monitoring" | ||
type = "MicrosoftMonitoringAgent" | ||
|
||
protected_settings = <<PROTECTED_SETTINGS |
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.
basic
should not contain optional properties: protected_settings
and settings
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.
fixed
|
||
* `type` - (Required) Specifies the type of the extension. For example `CustomScriptExtension` or `AzureMonitorLinuxAgent`. Changing this forces a new resource to be created. | ||
|
||
* `auto_upgrade_minor_version_enabled` - (Optional) Indicates whether the extension should use a newer minor version if one is available at deployment time. Once deployed, however, the extension will not upgrade minor versions unless redeployed, even with this property set to true. Possible values are `true` and `false`. |
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.
what are the differences between auto_upgrade_minor_version_enabled
and automatic_upgrade_enabled
?
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.
The auto_upgrade_minor_version_enabled
is about minor version, and it only works in creation stage. This two properties are available in similar resource like azurerm_virtual_machine_extension
-> **NOTE:** Possible values for `type_handler_version` can be found using the Azure CLI, e.g.: | ||
|
||
```shell | ||
az vm extension image list --publisher Microsoft.Azure.Monitor -n AzureMonitorWindowsAgent --location westus -o table |
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.
Shall we provide a link to this Azure CLI instead of directly providing the shell command?
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.
Removed because this cannot be found in doc, and it might be inaccurate.
Hi @ms-zhenhua , thanks for reviewing this, please kindly take another look.
|
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 @teowa ,
Thank you for your updates. LGTM~
@teowa - could we fix up the merge conflicts please? |
…m into stackhci_extension
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.
LGTM 🌳
Community Note
Description
new resource
azurerm_stack_hci_extension
azure doc: https://learn.microsoft.com/en-us/azure-stack/hci/manage/arc-extension-management?tabs=azureportal
rest api: https://github.com/Azure/azure-rest-api-specs/blob/c7b98b36e4023331545051284d8500adf98f02fe/specification/azurestackhci/resource-manager/Microsoft.AzureStackHCI/stable/2024-01-01/extensions.json
PR Checklist
For example: “
resource_name_here
- description of change e.g. adding propertynew_property_name_here
”Changes to existing Resource / Data Source
Testing
Tests depends on Stack HCI Arc Setting resource generated during HCI deployment, the HCI deployment steps is included in PR #25646, my local tests can pass with a existing Arc Setting resource.
Change Log
Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.
azurerm_stack_hci_extension
This is a (please select all that apply):
Related Issue(s)
Fixes #0000
Note
If this PR changes meaningfully during the course of review please update the title and description as required.