search

Azure / template-specs

MIT License
31 stars 4 forks source link

PSTemplateSpecSingleVersion should not exist - There should only be PSTemplateSpecMultiVersion #27

Closed ChristopherGLewis closed 4 years ago

ChristopherGLewis commented 4 years ago

Using Get-AzTemplateSpec with a -version parameter returns two different powershell objects - this is against most good powershell practices.

Get-AzTemplateSpec should always return an object with Versions as an array, then let Powershell handle the Array. If there is one array element, .Version would return the first (along with .Version[0]). Multiple Versions would result in Versions[0], Versions[1] etc.

You could add an additional method Get-AzTemplateSpecVersion -name -resourcegroup -version that just returns a PSTemplateSpecVersion object.

This would be very similar to the Get-AzVirtualNetwork Vnet/Subnet relationship.

stuartko commented 4 years ago

We need to support:

Currently foreseen options:

Given all the necessary cases needing support, the current implementation (having a base PSTemplateSpec class, with PSTemplateSpecMultiVersion/PSTemplateSpecSingleVersion inheriting from it) was created to satisfy all the conditions while trying to minimize the cons. As this is private preview we're certainly open to discussing alternatives.

Our cmdlets were designed/reviewed with the Powershell team's guidance, and their guidance at the time was that alternate types could be returned, as long as they shared the same base class which contained data relevant to the action... Following up on the customer feedback/requests to have a single type returned, to better understand the issue, is this due to it potentially breaking some of your scenarios? Or is the desire moreso to stay consistent with what is generally seen in other cmdlet implementations?

ChristopherGLewis commented 4 years ago

I believe that B is the only "sane" way to process this - since PowerShell can automatically collapse arrays and given the vast examples of returning array of 0-N objects in the AZ module I don't think this is an issue to the general Az Powershell base.

For the most part, Powershell/Az programmers have no issues with the concepts of arrays and empty arrays. One of the fundamental concepts in Azure is Tags, and every object in the AZ namespace has an array of 0-n tags.

I listed vnet/subnet as an example, but there are plenty of others:

The thing I'm confused about is that behind the scenes it looks like PSTemplateSpecMultiVerision and PSTemplateSpecSingleVersion are just a wrapper for a single vs array of PSTemplateSpecVersion objects. There's no difference in the end object, so I don't see why this is even a question.

decompiling:

public PSTemplateSpec GetTemplateSpec(string templateSpecName, string resourceGroupName, string templateSpecVersion = null)
{
    TemplateSpec templateSpec = GetAzureSdkTemplateSpec(resourceGroupName, templateSpecName);
    if (templateSpecVersion == null)
    {
        List<TemplateSpecVersion> allVersions = new List<TemplateSpecVersion>();
        IPage<TemplateSpecVersion> versionPage = TemplateSpecsClient.TemplateSpecVersions.List(resourceGroupName, templateSpecName);
        allVersions.AddRange(versionPage);
        while (versionPage.NextPageLink != null)
        {
            versionPage = TemplateSpecsClient.TemplateSpecVersions.ListNext(versionPage.NextPageLink);
            allVersions.AddRange(versionPage);
        }
        return new PSTemplateSpecMultiVersion(templateSpec, allVersions.ToArray());
    }
    TemplateSpecVersion specificVersion = TemplateSpecsClient.TemplateSpecVersions.Get(resourceGroupName, templateSpecName, templateSpecVersion);
    return new PSTemplateSpecSingleVersion(templateSpec, specificVersion);
}
internal PSTemplateSpecSingleVersion(TemplateSpec templateSpecModel, TemplateSpecVersion versionModel)
    : base(templateSpecModel)
{
    Version = PSTemplateSpecVersion.FromAzureSDKTemplateSpecVersion(versionModel);
}

VS

internal PSTemplateSpecMultiVersion(TemplateSpec templateSpecModel, TemplateSpecVersion[] versionModels)
    : base(templateSpecModel)
{
    if (versionModels == null)
    {
        Versions = new PSTemplateSpecVersion[0];
        return;
    }
    Versions = versionModels.Select((TemplateSpecVersion v) => PSTemplateSpecVersion.FromAzureSDKTemplateSpecVersion(v)).ToArray();
}

I think your underlying assumption that it would be confusing and hard to maintain is not really the issue you state it is.

I do believe that retrieving all the versions of a TemplateSpec might be expensive as you've indicated, but there are other Az objects that do this (i.e. Get-AzVMImage and much of the new ResourceGraph work).

Thoughts?

stuartko commented 4 years ago

> "it looks like PSTemplateSpecMultiVersion and PSTemplateSpecSingleVersion are just a wrapper for a single vs array of PSTemplateSpecVersion objects"

That's correct. The difference is that MultiVersion contains multiple versions in a property named "Versions" and SingleVersion contains a single version in a "Version" property of the Template Spec object.

> I do believe that retrieving all the versions of a TemplateSpec might be expensive as you've indicated, but there are other Az objects that do this (i.e. Get-AzVMImage and much of the new ResourceGraph work).

I think for "most" users it would not be prohibitively expensive, but for enterprise customers we often see very large templates which can be further compounded if they are using a publishing pipeline, and for template specs we could be looking at potentially 10s or 100s of MBs of response data for a single listing at a subscription scope.. I'd like to avoid that scenario.

Regarding merging Multi/Single Version into a single type, I'd like to discuss this further internally. If we do this we'd likely name it something like "PSTemplateSpecWithVersion" (open to alternate naming suggestions too of course). And support the following required scenarios:

There's also the possibility of adding ".Versions" to PSTemplateSpec and using the same behavior as above, but there are some additional caveats of that approach that would need to be thought out further.

Going back on the request for the change, I want to get a better understanding of what limitations using two types inheriting from PSTemplateSpec imposes on powershell cmdlet users such as yourself. Do you see any technical issues it causes for your scripts? Or is it more of a "few other cmdlets do it this way" type objection? Understanding this will help with prioritization.

stuartko commented 4 years ago

FYI / Update: After consulting further with the Powershell team, we're going to be moving forward with a unified type. Our existing method was within the original Powershell guidance, but due to multiple customer requests and feedback items (such as this issue) for a unified type, we want to ensure we deliver an optimal solution to customers. PSTemplateSpec will gain a ".Versions" property which will be populated with the following scenarios/values:

The caveat here is that there won't be a differentiation between a template spec requested that only had a single version, and a template spec that was requested with a specific version. In both cases .Versions will have a single item and will be displayed/formatted as "Versions: versionNameHere" when the object is displayed by the default PSTemplateSpec formatter.

@ChristopherGLewis : Thoughts / Feedback?

stuartko commented 4 years ago

Closing this issue as it has been addressed by the latest release: v0.1.8