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

Deploying multiple resource groups #466

Closed
wants to merge 22 commits into from

Conversation

r30e
Copy link
Contributor

@r30e r30e commented Nov 18, 2020

This PR closes #215 #457

The changes in this PR are as follows:

  • ArmBuilder renamed to ResourceGroupBuilder (arm kept as an alias for backwards-compatibility)
  • Added SubscriptionDeploymentBuilder
  • Added Deployment helper module
  • All arm deployments are now wrapped in subcriptionDeployments so that creating the resourceGorup is part of the template
  • Deploy.whatIf and Deploy.validate no longer create resource groups before executing

I have read the contributing guidelines and have completed the following:

  • Tested my code end-to-end against a live Azure subscription.
  • Updated the documentation in the docs folder for the affected changes.
  • Written unit tests against the modified code that I have made.
  • Updated the release notes with a new entry for this PR.
  • Checked the coding standards outlined in the contributions guide and ensured my code adheres to them.

If I haven't completed any of the tasks above, I include the reasons why here:

@r30e r30e marked this pull request as ready for review November 21, 2020 16:00
@r30e
Copy link
Contributor Author

r30e commented Nov 21, 2020

This also fixes #457

@isaacabraham isaacabraham added this to the 1.4 milestone Dec 12, 2020
@isaacabraham
Copy link
Member

@TheRSP Just an update on this. I'm finishing off the other PRs for 1.4 today and this week and then will review this. It's a significant change so it may be that we push it back to 1.5 or even 2.0, but I want to take the time to go through what you've done first before making a judgement call on that.

Thanks

@r30e
Copy link
Contributor Author

r30e commented Feb 1, 2021

@isaacabraham Thanks for the update. Good to know it's still on the to-do list. I'll try and fix the merge on Friday

@isaacabraham isaacabraham modified the milestones: 1.4, v2 Feb 7, 2021
@isaacabraham
Copy link
Member

@TheRSP Just going through this now, bringing it up to latest master. I can see the benefits of doing this - even if we weren't to expose this to end users, moving to this means that the template contains more than before and means less reliance on the CLI for specific arguments e.g. resource group created within the deployment, incremental deployment etc.

I'm going to make a PR for my merge into this branch because I'll need you to validate what I've done before putting it in here. Some thoughts so far:

  • There are some dependencies in the test project that seem very old - 2016 - and are not compatible with netstandard much less net50. Can we find alternatives for these?
  • Parameters need to be changed so that they are emitted in the "template" element of ARM rather than as a top level subscription parameter.
  • Clearly, the output of the ARM template is quite different from now - I'm trying to avoid breaking changes to the output format but this might be OK.

I'll push the PR later today.

# Conflicts:
#	Farmer.sln
#	RELEASE_NOTES.md
#	docs/content/api-overview/resources/arm.md
#	src/Farmer/Common.fs
#	src/Farmer/Deploy.fs
#	src/Farmer/Farmer.fsproj
#	src/Tests/Tests.fsproj
#	src/Tests/test-data/lots-of-resources.json
@isaacabraham
Copy link
Member

@TheRSP interestingly, I was looking at this just now for to see what would be required for a minimal implementation that would generate resource groups within the template, but not change the surface area API. I found something interesting:

It seems that you can deploy resources at the "top" level in a flat structure rather than embedding the template inside. See here.

Any reason we couldn't use this approach? It looks a bit clearer than the nested version.

@r30e
Copy link
Contributor Author

r30e commented Feb 23, 2021

The 'Deploy to target resource group' section from that link, I believe is a standard resource group deployment. Unless I'm mistaken, this is what farmer does today. So I don't believe that would support deploying resource groups or subscription resources.

@isaacabraham
Copy link
Member

@TheRSP Mmmm. You're right. Not sure what I was thinking there!

Either way - I created a basic prototype (more for my learning than anything) just to see if I could get this working using the current Farmer surface API i.e. using a subscription-level deployment with a nested template into a single resource group. I actually think that that would be a nice stepping stone to introducing full multi-resource group capabilities (which, I must admit, feels like a bit of an edge case to me, but I might be completely wrong there).

The main thing I've changed is that the resource group name in the generated template is parameterised - this way you can redeploy the same template to multiple resource groups rather than embedding the resource group name directly in the template.

The only issue I've been struggling with is passing parameters to a nested template. According to the docs, you can set a parameter at the "top" level and pass it into the nested template, but this didn't work. Not sure how to do it.

@r30e
Copy link
Contributor Author

r30e commented Mar 8, 2021

@isaacabraham sorry for the delay in getting back to you.
The issue is probably that we're using scope: inner in expressionEvaluationOptions - as a result of this, a reference to [parameters('foo')] will only look at the inner deployment and not the outer deployment. I believe this can be solved by adding a parameters property to the deployment resource properties which copies the external parameters into the inner deployment:

    {
      "apiVersion": "2020-06-01",
      "dependsOn": [
        // ...
      ],
      "properties": {
        "expressionEvaluationOptions": {
          "scope": "inner"
        }, 
        // vvvvvvvvvvvvvvvvvv
        "parameters": {
            "StorageAccountName": {
                "value": "[parameters('StorageAccountName')]"
            }
        },
        // ^^^^^^^^^^^^
        "mode": "Incremental",
        "template": {
          "$schema": "https://schema.management.azure.com/schemas/2019-04-01/deploymentTemplate.json#",
          "contentVersion": "1.0.0.0",
          "outputs": {},
          "parameters": {},
          "resources": [
            // ...
          ],
          "variables": {}
        }
      },
      "type": "Microsoft.Resources/deployments"
    }

Similar case example available here: https://samcogan.com/modularisation-and-re-use-with-nested-arm-templates/ in the section "Nested Files"

@isaacabraham
Copy link
Member

@TheRSP urgh :-) OK, I'll give that a bash :-)

@r30e
Copy link
Contributor Author

r30e commented Apr 27, 2021

This branch has now diverged too far from master to easily deal with the conflicts. As such, I shall abandon this and try to achieve the result with a series of smaller PRs which will hopefully be easier to merge

@r30e r30e closed this Apr 27, 2021
@r30e r30e mentioned this pull request Apr 30, 2021
5 tasks
@r30e r30e deleted the subscription-level-2 branch July 19, 2021 06:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for resource groups at the subscription level
2 participants