-
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
automation
: update sdk of remain resources to 2022-08-08
#22989
Conversation
automation
: update sdk of remains resources to 2022-08-08automation
: update sdk of remain resources to 2022-08-08
internal/services/automation/automation_hybrid_runbook_worker_group.go
Outdated
Show resolved
Hide resolved
"github.com/hashicorp/terraform-provider-azurerm/internal/common" | ||
) | ||
|
||
type Client struct { | ||
AccountClient *automationaccount.AutomationAccountClient | ||
*v20220808.Client |
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 give this a name e.g.
*v20220808.Client | |
V20220808 *v20220808.Client |
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 this guideline mandatory? because I see network
is using the embedding too. additionally I believe the embedded form improves readability and facilitates future SDK upgrades.
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.
There might need to be some internal discussion on what the convention is there - if we're going with how the meta client is defined in network then we should at least follow the naming convention for the import there which is
network_2023_04_01.Client
, so in this case automation_2022_08_08.Client
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.
updated. But I think It's a little bit wordy for automation
service's sdk with a automation
prefix. and golang style guide suggests rename the import package name with no underscore.
Go package names should not have underscores. If you need to import a package that does have one in its name (usually from generated or third party code), it must be renamed at import time to a name that is suitable for use in Go code.
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 agree with you that since the prefix is already automation
then there's duplication which could be removed there.
Since this was introduced by @tombuildsstuff there could be a specific reason for doing so which we're not aware of yet (can't check either since he's out at the moment), rather than introduce a different standard into the provider it's better to follow the status quo which will make updating it further down the line quicker since you're only searching for one pattern.
I hope that makes sense.
return &Client{ | ||
AccountClient: accountClient, | ||
Client: metaClient, |
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.
Client: metaClient, | |
V20220808: metaClient, |
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.
Tests look good, thanks @wuxu92 LGTM 👍
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. |
also use a meta client for automation.