Added Azure Credentials Manager Singleton

[estebanreyl]

Restructured PR, contains only essential files for this functionality.
Created a unified client in a singleton to facilitate acquiring the azure account, subscriptions, resource groups, registries etc. from a unified simple location.

-An important note is that this PR is a dependency for further code so this is fairly essential.

Note that I have not refactored existing code to fit the new singleton, this will not be done as other code works well and to avoid any possible introduction of bugs. Any feedback would be highly appreciated before opening PR on Microsoft branch.

[northtyphoon]

A couple of code style comments. LGTM.

[northtyphoon]

call this.getAccount().filters... so you can have the consistent check.

[northtyphoon]

nit: registry let me think it's single registry

[northtyphoon]

just return loggedIntoAzure ?

[northtyphoon]

What is the code for //ERR CHECKS?

[estebanreyl]

/ERR CHECKS is meant to be a section whose only member is isLoggedIn()
Is this considered bad style?

[northtyphoon]

I see. I just feel a bit of odd to categorize login check to err check.

[estebanreyl]

Ive just put it under CHECKS which seems more reasonable

[ankurkhemani]

Your code is correct but as a more general implementation of a Singleton pattern, you can make the constructor private instead of doing manual check and do a lazy initializtion of class instance in the getInstance method?

class Singleton {
    private static _instance: Singleton;
    private constructor() {
        // constructor made private to control object creation
    }
    public static getInstance(): Singleton {
        if (!Singleton._instance) { // lazy initialization
            Singleton._instance = new Singleton();
            // ... any other one time initialization goes here ...
        }
        return Singleton._instance;
    }
}

[estebanreyl]

Thanks for the feedback! I should have done that, I am more accustomed to this pattern from Javascript due to the lack of private. Ill update that by early tomorrow.

[ankurkhemani]

Looks like this method will be used internally and can be made private?

[estebanreyl]

That method is meant to be for external use, maybe providing a different name/ providing additional filtering and labeling it getSubscriptions where this particular filtering is default would be better?

[ankurkhemani]

You can keep it public if intended for external use. I think naming is fine the way it is and can be modified in the future if needed.

[ankurkhemani]

nit: You can also check for registries not empty or better has size > 1.

[ankurkhemani]

nit: You can return here instead of creating a new const.

[ankurkhemani]

nit: Let's use more verbose names: s->azureSession, i->index ? Also, do we need to declare unused i and array ?

[estebanreyl]

I'll get back to you on that, getCredentialsByTenantId was not implemented by me, rather it was a method existent in preexisting code from another class in the project (I think azureRegistryRootNode.ts). Ill take a look at it and implement your suggestions though as that does seem clearer.

[ankurkhemani]

Should we abstract this to a method getResourceManagementClient?

[ankurkhemani]

nit: you can re-use method getContainerRegistryManagementClient here.

[ankurkhemani]

LGTM, some nit comments that can be addressed.

[ankurkhemani]

You can remove this: unreachable code.

[estebanreyl]

Would I just maintain an empty private constructor then?

[ankurkhemani]

Yes, making the constructor private ensures this class cannot be instantiated. Hence, the only way is to use static object creation getInstance where a singleton instance is created only the first time its called. Singleton is just a fancy way of using a global shared object.

[ankurkhemani]

Do no initialize here, defeats the purpose of lazy initialization.

[estebanreyl]

Oh that's my bad, I thought Id gotten that fixed.