-
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
azurerm_recovery_services_protection_policy_vm: add timezone property #2404
Conversation
b12dff4
to
f5ca502
Compare
f5ca502
to
d485c04
Compare
d485c04
to
37e6593
Compare
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.
otherwise LGTM 👍
@@ -217,7 +217,8 @@ func resourceArmRecoveryServicesProtectedVmWaitForState(client backup.ProtectedI | |||
|
|||
resp, err := state.WaitForState() | |||
if err != nil { | |||
return resp.(backup.ProtectedItemResource), fmt.Errorf("Error waiting for the Recovery Service Protected VM %q to be %t (Resource Group %q) to provision: %+v", protectedItemName, found, resourceGroup, err) | |||
i, _ := resp.(backup.ProtectedItemResource) |
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.
imo we should be raising this error, since i
won't be valid if it's non-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.
I am raising it as part of the errorf? the cast is done like this because sometimes resp ends up being 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.
sorry, I was referring to the casting error that's a _
?
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 the reasoning behind this is if resp is nil
i := resp.(backup.ProtectedItemResource)
fails.
however if you use the ok
cast, even if its _
for unused
i, _ := resp.(backup.ProtectedItemResource)
the cast doesn't not fail on a nil value and I becomes the zero value for (backup.ProtectedItemResource): https://play.golang.org/p/cDZrRXP6C27
"timezone": { | ||
Type: schema.TypeString, | ||
Optional: true, | ||
Default: "UTC", |
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.
should we apply some validation to this?
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.
It would be nice, however I don't know what of the many different timezone formats is valid here (there are at least 2 different sets of valid values it could be, maybe more)
Co-Authored-By: katbyte <[email protected]>
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! |
fixes #2147