Skip to content
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

Allow customization of naming patterns for resources created with install.instance #126

Closed
sobjornstad opened this issue May 26, 2020 · 10 comments
Assignees
Labels
enhancement New feature or request fixed Bug got fixed or found a solution waiting issuer Waiting information or data from original issuer

Comments

@sobjornstad
Copy link

Feature Request

Is your feature request related to a problem? Please describe.
Our organization has standard naming conventions for resources in Azure. Being able to provide only a single resource name to prefix all the resources the Aggregator creates (functions app, storage account, App Insights, App Service plan) is not sufficient to meet these naming conventions, which require specific prefixes for each type of resource.

Describe the solution you would like
Add options to individually name each of the resources created by the installation, or at least to alter them after the fact and have further operations continue to work. While we're at it, removing the requirement to have aggregator suffixed to the name might be nice (this one isn't an issue for me, but it might bother others).

Describe alternatives you have considered
I cannot think of any other way to solve this, but perhaps there would be some hacks possible if I knew more about how the Aggregator worked. Please let me know if you know of them.

@sobjornstad sobjornstad added the enhancement New feature or request label May 26, 2020
@giuliov
Copy link
Member

giuliov commented May 27, 2020

Thanks Soren for sharing your requirements. I fully understand the scenario and it would be easy to implement (naming algorithm is in InstanceName.cs and instance-template.json).
What blocks me from coding is that removing the aggregator string is a breaking change. Surfacing breaking changes is hard: even advertising using a big bold font is not sufficient.
Any suggestion for managing the transition is welcome.

@BobSilent
Copy link
Collaborator

I would provide a default behaviour, with the old prefix and keep the command line arguments for the default users.
And describe handling of the template file for experienced users, they do not provide the command line arguments but e.g. a "from naming template" argument, with the possibility to provide each name separately

@giuliov
Copy link
Member

giuliov commented May 27, 2020

I like the idea of a naming template

I found four places where names are set

// src\aggregator-cli\Instances\InstanceName.cs:9-10
    const string resourceGroupPrefix = "aggregator-";
    const string functionAppSuffix = "aggregator";

// src\aggregator-cli\Instances\instance-template.json:63
    "storageAccountName": "[concat('aggregator', uniquestring(resourceGroup().id))]",

// src\aggregator-cli\Instances\AggregatorInstances.cs:189
    string deploymentName = SdkContext.RandomResourceName("aggregator", 24);

// src\aggregator-cli\Mappings\AggregatorMappings.cs:41
    && s.ConsumerInputs.GetValue("url","").IndexOf("aggregator.azurewebsites.net") > 8);

The first three set resource name while creating, so easy peasy.
The latter is used to find all WebHook Subscriptions in Azure DevOps.

Looks doable

@sobjornstad
Copy link
Author

I like the naming-template idea as well, and was definitely thinking a new command-line option, not a change to the existing one. I don't see any reason to change the default behavior to remove aggregator -- it's probably more helpful than anything for people who aren't in a complex enterprise environment.

giuliov added a commit that referenced this issue Jun 17, 2020
* refactor to support custom naming templates (ref #126)

* cleaning Naming interface

* refactored for custom naming template

and initial set of unit tests

* more unit tests

* convert ARM template to use additional parameters

* rename to better convey

* fix ARM template sanity check

* editorconfig, attributes and using clean-up

* fixed implementation of FileNamingTemplates

added more argument validation
added unit tests

* Integration test for naming template feature

and some fixes
and some doc notes

* fix integration test cleanup

should be moved to a special test command

* factored out integration test cleanup

that is delete all azure resources except for the resource group
@giuliov
Copy link
Member

giuliov commented Jun 18, 2020

Feature implemented in new 0.9.11 release. @sobjornstad, would you like to have a look?

@giuliov giuliov self-assigned this Jun 18, 2020
@giuliov giuliov added the waiting issuer Waiting information or data from original issuer label Jun 18, 2020
@sobjornstad
Copy link
Author

Will do! I just got back from vacation -- I'll try to have a look at it this week.

@sobjornstad
Copy link
Author

sobjornstad commented Jun 29, 2020

This is looking good!

I'm puzzled about one thing, though: even though --resourceGroup has to be explicitly specified when using a naming template, one is still required to put a non-empty ResourceGroupPrefix or ResourceGroupSuffix into the naming template, and then one must specify only a part of the actual resource group name for --resourceGroup (so that the correct name gets put together by the prefix/suffix and the part supplied on the command line). What is the purpose of having prefix/suffix configuration options for resource groups at all when the resource group name won't be automatically generated anyway?

Not hard to get around, but it doesn't make much sense to me.

@giuliov
Copy link
Member

giuliov commented Jul 10, 2020

It is due to some assumption in some code. I will explore if I can get rid of those.

@giuliov
Copy link
Member

giuliov commented Jul 23, 2020

Removed the limitation in 0.9.13. Now you can leave ResourceGroupPrefix and ResourceGroupSuffix out.

@giuliov
Copy link
Member

giuliov commented Aug 6, 2020

Released in 0.9.14

@giuliov giuliov closed this as completed Aug 6, 2020
@giuliov giuliov added the fixed Bug got fixed or found a solution label Aug 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request fixed Bug got fixed or found a solution waiting issuer Waiting information or data from original issuer
Projects
None yet
Development

No branches or pull requests

3 participants