-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Support for Azure Batch - Account #2428
Support for Azure Batch - Account #2428
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.
hey @jcorioland
Thanks for opening this PR - whilst I appreciate this is still a WIP I took a look through and left a few comments inline, but this is off to a good start 👍
Thanks!
azurerm/data_source_batch_account.go
Outdated
ValidateFunc: validation.StringInSlice([]string{ | ||
string(batch.BatchService), | ||
string(batch.UserSubscription), | ||
}, true), |
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 think this field should just have Type+Computed?
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.
Hmm, I am not sure to understand why this field should be marked as Computed? It should be specified in the configuration.
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.
Ok sorry, I was looking on the resource, not the datasource :-)
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.
so this is specific to the Data Source where whilst this information will be returned it can't be set (since we only obtain the Batch Account using the Name & Resource Group - the other fields are outputs)
@jcorioland rathe than combining multiple resources into a single PR could we split these out into two different PR's, one for the Account and one for the Pool? |
7888ed0
to
ec101b5
Compare
@tombuildsstuff I was actually thinking the same to keep merge operations simple. I've created a new PR #2461 for Azure Batch pool. Not sure I did it well, tell me if I am wrong. |
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.
hey @jcorioland
Thanks for pushing those changes - I've taken a look through and left some minor comments in-line but this is mostly looking good, if we can fix those up we should be able to run the tests and get this merged 👍
Thanks!
log.Printf("[INFO] preparing arguments for Azure Batch account update.") | ||
|
||
resourceGroupName := d.Get("resource_group_name").(string) | ||
name := d.Get("name").(string) |
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.
outside of the Create function we should parse these from the ID - as such can we make this:
id, err := parseAzureResourceID(d.Id())
if err != nil {
return err
}
name := id.Path["batchAccounts"]
resourceGroupName := id.ResourceGroup
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.
done.
azurerm/data_source_batch_account.go
Outdated
}, | ||
"pool_allocation_mode": { | ||
Type: schema.TypeString, | ||
Optional: true, |
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.
since this is the Data Source this field isn't set-able, so we can remove the Optional
here
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.
done.
@tombuildsstuff - thanks for the review! I have done the updates. |
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 @jcorioland,
Thank you for the quick updates! I've taken a look through and left some additional minor/stylistic comments. It also seems like you may have missed some of tom's in the previous review, github can be sneaky and hide them under a "14 hidden conversations / Learn More" banner.
|
||
err = future.WaitForCompletionRef(ctx, client.Client) | ||
// if the error is not that the Batch account was not found | ||
if err != nil && future.Response().StatusCode != http.StatusNotFound { |
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.
Same
if err = future.WaitForCompletionRef(ctx, client.Client); err != nil {
if !response.WasNotFound(future.Response()) {
return fmt.Errorf("Error waiting for deletion of Batch account %q (Resource Group %q): %+v", name, resourceGroupName, err)
}
}
@katbyte @tombuildsstuff - thank you both for the reviews and sorry for missing a huge part of Tom's review the other day :) Should be good now! |
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.
Thanks for the quick updates again @jcorioland, aside from some minor indentation issues this LGTM 👍
(looks like i don't have permission to push to the branch)
@katbyte - thanks for having approved this PR. I've done the few updates you've asked for. It's weird that you were not able to push on the branch, I've checked and the |
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.
hey @jcorioland
Thanks for pushing those changes - ignoring a couple of minor comments (which I'm going to push a commit for) this now LGTM 👍 - I'll kick off the tests shortly :)
Thanks!
}) | ||
} | ||
|
||
func TestAccAzureRMBatchAccount_noStorageAccount_basic(t *testing.T) { |
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.
we should update this:
func TestAccAzureRMBatchAccount_noStorageAccount_basic(t *testing.T) { | |
func TestAccAzureRMBatchAccount_basic(t *testing.T) { |
|
||
* `pool_allocation_mode` - (Optional) Specifies the mode to use for pool allocation. Possible values are `BatchService` or `UserSubscription`. Defaults to `BatchService`. | ||
|
||
* `storage_account_id` - (Optional) Specifies the storage account to use for the Batch account. If not specified, Azure Batch will manage the storage. |
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.
we can remove this, since it's specified in the argument reference above
* `storage_account_id` - (Optional) Specifies the storage account to use for the Batch account. If not specified, Azure Batch will manage the storage. |
|
||
* `id` - The Batch account ID. | ||
|
||
* `pool_allocation_mode` - (Optional) Specifies the mode to use for pool allocation. Possible values are `BatchService` or `UserSubscription`. Defaults to `BatchService`. |
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.
we can remove this since it's specified in the argument reference above
* `pool_allocation_mode` - (Optional) Specifies the mode to use for pool allocation. Possible values are `BatchService` or `UserSubscription`. Defaults to `BatchService`. |
Hi @tombuildsstuff awesome, thank you! |
Hello @tombuildsstuff @jcorioland |
Hi @phosphre, Thanks for bringing this to our attention, but could you open a issue please? It was only by chance i happened to see this comment as we don't usually review closed PRs/ Thanks! |
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! |
This PR will brings support for Azure Batch Account.
Will partially fix #2427. Needs to be merged before #2461
Azure Batch Account: