-
Notifications
You must be signed in to change notification settings - Fork 0
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
DTL Builder #4
DTL Builder #4
Conversation
…thod and start testing
… need to port some of this
@@ -281,7 +281,7 @@ func buildAuthorizer(ctx context.Context, authOpts NewSDKAuthOptions, env enviro | |||
var authConfig auth.Credentials | |||
switch authOpts.AuthType { | |||
case AuthTypeDeviceLogin: | |||
return nil, fmt.Errorf("DeviceLogin is not supported, however you can use the Azure CLI `az login --use-device-code` to use a device code, and then use CLI authentication") | |||
return nil, fmt.Errorf("DeviceLogin is not supported in v2 of the Azure Packer Plugin, however you can use the Azure CLI `az login --use-device-code` to use a device code, and then use CLI authentication") |
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.
Are these changes from another branch? Or do you need to rebase onto main. These changes pertain to the arm builder and not DTL.
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.
While adding the dtl/azure_client
I thought it would be better to clarify this error message, I made the same change on the ARM client for consistency
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.
This looks good me. Just a couple of questions regarding some ARM builder changes.
@@ -95,7 +102,7 @@ func (b *Builder) Run(ctx context.Context, ui packersdk.Ui, hook packersdk.Hook) | |||
return nil, err | |||
} | |||
if b.config.ClientConfig.ObjectID == "" { | |||
b.config.ClientConfig.ObjectID = getObjectIdFromToken(ui, spnCloud) | |||
b.config.ClientConfig.ObjectID = *objectId |
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.
This might've come up in a different review but do we need to nil check objectId?
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 it did but I forgot to respond my bad!
I think we don't because of how the upstream method works, NewAzureClient returns a pointer to a string that we instantiate, if that's null it should just resolve to an empty string, then below this we check if object ID is set to empty, and alert the user that "could not determine the ObjectID for the user, which is required for WIndows builds", we don't fail the builder if we can't determine the object ID, although we probably should for Windows builds as it is required
if err == nil { | ||
err = f.WaitForCompletionRef(ctx, azureClient.DtlCustomImageClient.Client) | ||
} | ||
err := azureClient.DtlMetaClient.CustomImages.DeleteThenPoll(ctx, customImageResourceId) |
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.
Nice!
This PR Ports the DTL Builder to the new SDK.
A lot of unused code was copied over when this builder was made for the ARM builder, specifically related to VHD builds which the DTL Builder has never supported.
This PR does not add new DTL features