-
Notifications
You must be signed in to change notification settings - Fork 79
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 license type for Azure Hybrid Benefit #248
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.
Looks good to me
errs = packersdk.MultiErrorAppend(errs, fmt.Errorf("The license_type %q is invalid for os_type %q", c.LicenseType, c.OSType)) | ||
} | ||
} else if c.OSType == constants.Target_Windows { | ||
if strings.EqualFold(c.LicenseType, constants.License_Windows_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.
you are just trying to make sure the license type is of certain valid values, so this code can be simplified to
if(!strings.EqualFold(c.LicenseType, constants.License_Windows_Client) &&
!strings.EqualFold(c.LicenseType, constants.License_Windows_Server))
{
errs = packersdk.MultiErrorAppend(errs, fmt.Errorf("The license_type %q is invalid for os_type %q", c.LicenseType, c.OSType))
}
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's also normalizing the case of LicenseType to avoid any case sensitivity issues with the Azure API. (Similar to the OS parameter)
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.
makes sense!
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.
Code is looking good so far, made some suggested changes to the error messaging let me know what you think!
@@ -325,6 +325,22 @@ | |||
this will set the prefix for the resources. The actuall resource names will be | |||
`custom_resource_build_prefix` + resourcetype + 5 character random alphanumeric string | |||
|
|||
- `license_type` (string) - Specify a license type for the VM to enable Azure Hybrid Benefit. If not set, Pay-As-You-Go license |
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.
Very nice
Co-authored-by: Jenna Goldstrich <[email protected]>
Co-authored-by: Jenna Goldstrich <[email protected]>
Good suggestions, I applied them. 😄 |
Hi guys, hope you could find some time to complete the PR. A lot of teams are blocked right now because of this issue. We will really appreciate your help here! |
Co-authored-by: Jenna Goldstrich <[email protected]>
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 was able to manually verify this, and run acceptance tests, worth noting the Packer team doesn't have an active account to test Red Hat/SUSE, but logically I think they should work, I tested Windows Server and Windows Client.
Thanks for this contribution @dknott-arcadis!
Adds support for specifying the license type for the builder virtual machine.
Closes #154