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

[Question/Feedback]: Report "breaking" changes when changing paths/names #3988

Closed
Palving opened this issue Sep 19, 2023 · 10 comments
Closed
Assignees
Labels
question Further information is requested

Comments

@Palving
Copy link

Palving commented Sep 19, 2023

Description

Hello. We currently have an service which daily fetches the bicep modules located in this repository, and we create modules on top of these which we expose to our developers. All these modules are stored in Azure Container Registry.

However, we are experiencing a lot of breaking changes when paths or filenames are renamed, i.e
deploy.bicep -> main.bicep
microsoft.web -> web
microsoft.documentdb -> document-db

We could change things on our side, however the main issue is the uncertanity. Can we expect the current folder structure and namings to be the same going forward?

@Palving Palving added the enhancement New feature or request label Sep 19, 2023
@AlexanderSehr AlexanderSehr self-assigned this Sep 19, 2023
@AlexanderSehr AlexanderSehr added question Further information is requested and removed enhancement New feature or request labels Sep 19, 2023
@AlexanderSehr AlexanderSehr changed the title [Feature Request]: Report "breaking" changes when changing paths/names [Question/Feedback]: Report "breaking" changes when changing paths/names Sep 19, 2023
@AlexanderSehr AlexanderSehr pinned this issue Sep 19, 2023
@Palving
Copy link
Author

Palving commented Sep 20, 2023

In addition, we use the paths to create names for our images in OCI. Naming like this, "config--appsettings" breaks az publish,

Start publish on [resourcemodules/web/site/config--appsettings] version [2023-09-20] to registry [****bicepregistrydevcr]

ERROR: The specified OCI artifact reference "br:****bicepregistrydevcr.azurecr.io/resourcemodules/web/site/config--appsettings:2023-09-20" is not valid. The module path segment "config--appsettings" is not valid. Each module name path segment must be a lowercase alphanumeric string optionally separated by a ".", "_" , or "-".

@AlexanderSehr
Copy link
Contributor

Hey @Palving,
thanks for raising the question. I did also pin it as I think it's also relevant for others to see.
Before going into any details, let me first point you to a disclaimer that we added to the front page recently and should roughly set the stage.
As described there, we're currently hard at work to make the library 'AVM-compliant' which is a requirement in order to publish the modules to the Public Bicep Registry. How fun as this may sound - it comes with quite a few painful changes, some of which you already described in your initial comment (for example the file and folder name changes). For the upcoming period, we expect a landslide of further changes and hence created release 0.11.0 (and an upcoming 0.11.1) to mark a half-way, but stable, point in time. A module prepared for AVM will look a lot closer to the version in this branch as opposed to main.
As we're talking about moving the library into the Public Bicep Registry the question may arise why we introduce the breaking changes on the CARML side as opposed to the AVM/Public-Registry-side. And there are a few reasons for this:

  • We want to migrate the modules over as fast as possible and hence need to apply many changes on scale in the source (i.e. CARML)
  • We have to update the CARML CI simultaniously / shortly after, as the idea will be too be able to pull the modules from the Public Bicep Registry and inner-source them in the CARML CI as before. Hence the CI must be able to also work with the new names etc.

From where I am standing, I would recommend to pause your pipeline for the time being (especially if you're pulling from main as opposed to a stable release) until after the migration concluded. I cannot provide you with an exact timeline, but we hope to conclude the move before the end of the year. We're a bit dependent on the amount of changes we're expected to implement.

In any case I want to provide you with a few pointers as to what we know of today:

  • The module names should not change again
  • The file names should not change again
  • The readme will be updated (e.g., the parameters section will become a lot more detailed)
  • We'll add user defined types for the extension resources (i.e. rbac etc.) in a first step and update their interface (e.g., instead of individual diagnostic parameters, we'll have a diagnostics configuration object).
  • The test folder and its content will be renamed

We're currently working on a PoC for a handful of modules, so you should be able to see the result fairly soon. But again, until the migration is done, I would not recommend to pull in changes on the library-side. I hope that helps to leviate the uncertainty a bit.

@AlexanderSehr
Copy link
Contributor

AlexanderSehr commented Sep 20, 2023

In addition, we use the paths to create names for our images in OCI. Naming like this, "config--appsettings" breaks az publish,

Start publish on [resourcemodules/web/site/config--appsettings] version [2023-09-20] to registry [****bicepregistrydevcr]

ERROR: The specified OCI artifact reference "br:****bicepregistrydevcr.azurecr.io/resourcemodules/web/site/config--appsettings:2023-09-20" is not valid. The module path segment "config--appsettings" is not valid. Each module name path segment must be a lowercase alphanumeric string optionally separated by a ".", "_" , or "-".

Hello again @Palving,
that's an interesting behavior. Doublechecking I just noticed that we're actually not publishing those children due to the same error.
It will be a bit of a creative challenge to fix this and I'd be happy for any idea from your side. The reason we used the -- was to differentiate this module from classic 'child' modules. The child here is always config, but because the parameters are so different based on the type of the setting, the contributor essentially added variations of the module. And a 'variation' isn't an actual child, right? There is no resource type 'config-appsettings'.

Long story short, if you have an alternative idea we can go with that. I guess one of the other characters would also work (e.g., _ instead of the --), though it may look a bit alien 😄.

@Palving
Copy link
Author

Palving commented Sep 21, 2023

@AlexanderSehr Thanks for the quick reply, we'll adapt our current service and look forward to when all the changes are implemented :)

As for the variation modules, it really is a challenging issue, and I remember we had the same issue when we initially began working with bicep and creating our own modules.

Working with this repository, I've noticed kind of (but not really) the same issue with the roleassignments modules, where you have created a "main" main-bicep that refers to subfolders depending on the parameters of the main bicep.

i.e
roleAssignment/
----------> main.bicep
----------> resource-group/main.bicep

Could a similar approach be considering for the providers that works the same as config? This would have to work a bit differently than roleassignments, with each "variant" deploying their own type instead of being referenced from the "root"-bicep like in roleAssignment

config/
----------> web/main.bicep
----------> appsettings/main.bicep
----------> appsettings-v2/main.bicep

@AlexanderSehr
Copy link
Contributor

Hey @Palving,
it is indeed a bit different like you pointed out. The reasoning back there was that you cannot deploy into multiple different scopes using just one template (e.g., Subscription & ResourceGroup), so the Contributor created templates for each scope and invoked them from the 'parent'. The parent itself in a sense is quite odd / is not recommended to be used as it just ties the others together. There it makes a lot more sense to e.g. invoke the roleAssignment/resource-group/main.bicep template directly if you were to deploy a role assignment to a resource group.
With configs we did not have a scope, but a parameter set issue. I guess we could anyways use a similar approach, but either remains counter intuitive to the structure that we try to aspire to - that is - Parent->Child->Child->Child ... as per the ResourceType in Azure.

With the introduction of User Defined Types, we may get around this issue though. In case you're not familiar with that feature: It allows us to define the structure of an object / array. So we may actually be able to define different parameter sets in a single config module. Got to investigate this further - we're currently implementing the user defined types for the extension resources of this module, and expect some learnings from there.

@Palving
Copy link
Author

Palving commented Sep 26, 2023

User defined types may be the way to go, I cant see any other way to solve this that is not a rough workaround for the contract.

Another thing, this might be the wrong place to ask though so let me know if I should open another ticket. I found very little regarding this in the documenation (extenstion resources), but we've had some struggles with our larger bicep files that wraps the resource modules (10-15 resources deployed) because of the conditional resource deployments found in the resource modules. I.e private network, locks etc. We never deploy these, but when the bicep is converted to ARM they get brought along and can possibly create some very large ARM-templates. For example, if I were to deploy an app with roleassignments, I'd rather have a web/deploy.bicep that only deploys the web app, and a roleAssignment/deploy.bicep that deploys roleAssignments to principals to keep things separated and modular instead of conditionally handling roleAssignments via the web/deploy.bicep's parameters.

What is the reason behind this design pattern?

@AlexanderSehr
Copy link
Contributor

Hey @Palving,
all good. What you describe is actually the 'trade-off' that we ended up with. Over the years, we ran several PoCs to test different scenarios (similar to the one you described the keep the extension resources separate), but they did come at a heavy cost for maintainability (be it for testing, be it for keeping resources in sync, etc.).
To be fair in that PoC, we did not split them completely, but would have published a module that cannot deploy extension resources vs. one that can. They would still be linked though.
Keeping them completely separate would remove one of the pillars CARML is build on - that is, a one stop shop to deploy a resource with (optionally) all its features. In other words, it renders the extensions kinda meaningless as there would be no benefit to having them there. If one needs an RBAC deployment for a Storage Account they could then just as well implement it directly, right 😄 .
In any case - on the other side, you described the obvious problem - ARM templates grow in size, no matter if you're using the features or not. That in the face of the 4mb template size limit is a very present problem. I haven't had the case yet that a solution template would go on strike, even for 'bigger' solutions, but I can't say I haven't heard about it (ALZ-Bicep is a prime example). And I don't want to ruin your day - but this problem will become a whole lot bigger with User-Defined-Types. We're currently working on introducing them as part of the Azure-Verified-Modules effort, and a module like Cognitive-Services-Accounts just grew by 25% in lines.

Long story short, why did we end up with the pattern we have today:

  • It's easy to use (especially with user-defined-types) and ideally shortens/simplifies your solution templates
  • It's easier to test as we can test all templates of the modules via their test deployments - and even better - ensure they work together
  • It makes maintenance a whole lot easier, for example when it comes to versioning (note that the intend for those modules is to be published in a library)

To get around the template size issues you have a few options which should all be considered workarounds as there is no perfect solution:

  • Delete whatever you don't need from the modules. Yeah I know that sounds dumb as that introduces a whole lot of work if you were ever to pull from CARML again, but it 'is' a simple solution ;)
  • Cut your templates sizes. That's probably the worst, right? Having to change your solution template designs to cater to the modules rather than the other way around. Wanted to mention it regardless though.
  • Wait. Rumors say (and this is no guarantee as I really only heard about it), that the 4mb limit will be increased in the future. The ARM PG is well aware that this is a big issue, even more so with all the features they add to Bicep (e.g., the aforementioned User-defined-types, in-template-testing, metadata etc.). So there is an urge to do something about it. When this will happen I don't know. I only know that it's a technical limitation of the storage service used in the backend, so I can only guess it's not a simple fix. I've seen there is also an issue for it in the Bicep repository.

@Palving
Copy link
Author

Palving commented Sep 27, 2023

@AlexanderSehr Thank you for the in depth answers, it's given us a lot more insight into why things are the way they are.
We'll aim to continue to use this repository as our base modules into the future, letting you handle the maintenance and testing is quite valuable to us 😄

One approach we discussed if the size of the ARM-template should become problematic is the possibly of "blacklisting" certain modules from this repository, so that we can create our own versions of them with only the modules we need rather than pulling from here, but we'll cross that bridge when we get to it.

Thanks again

@Palving Palving closed this as completed Sep 27, 2023
@AlexanderSehr
Copy link
Contributor

@AlexanderSehr Thank you for the in depth answers, it's given us a lot more insight into why things are the way they are. We'll aim to continue to use this repository as our base modules into the future, letting you handle the maintenance and testing is quite valuable to us 😄

One approach we discussed if the size of the ARM-template should become problematic is the possibly of "blacklisting" certain modules from this repository, so that we can create our own versions of them with only the modules we need rather than pulling from here, but we'll cross that bridge when we get to it.

Thanks again

Hey @Palving,
happy to help. Feel free to use anything that provides value to you. Even if you were to not use the modules as-is at all, they can still be a reference of how some features can be deployed. I know from personal experience that, for example, Customer-Managed-Keys can be a real 'treat', so having a working example you can look at may already be some added value. No hard feelings ;)

@AlexanderSehr
Copy link
Contributor

Hey @Palving,
watching the latest Bicep Community Call, if just learned that one can now actually define 'ParameterSets' like in PowerShell for Bicep module parameters. As discussed for the initial issue, this should really help us out:

image

... while unfortunately not helping with the template size though. Still ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
Status: Done
Development

No branches or pull requests

4 participants