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

Example proposal for Issue #731 #1185

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dburriss
Copy link
Contributor

This is linked to the issue described in #731 and is just for demo purposes.

I have encountered this issue a few times with Farmer and do not know of an elegant solution.
Something like this can get you out in a pinch but it requires knowing what the builder is setting, which leaks the details. It's also just not very elegant.

let mutable myWebApp = webApp {
            name "myWebApp"
            service_plan_name "myServicePlan"
            setting "myKey" "aValue"
            sku WebApp.Sku.B1
            always_on
            app_insights_off
            worker_size Medium
            number_of_workers 3
            run_from_package
            system_identity
        }
        let disableAlwaysOn = false
        if disableAlwaysOn then 
            myWebApp <- { myWebApp with WebAppConfig.CommonWebConfig.AlwaysOn = true }

An elegant solution would be if we could implement For and Zero in a way that allowed for if in the CE but playing around with it for a few minutes did not work as expected. I have not worked with CEs for a while but not sure if this is even possible for a non-collection based CE.

A simple solution (albeit quite some work) is to take in a value but add an optional parameter.

let expected = false
let template = webApp {
  name "web"
  always_on expected
}

This matches what I usually do in builders in C# where boolean switching takes in a boolean with a default value.

@mattgallagher92
Copy link
Member

Thanks @dburriss! I like this approach 🙂 Particularly because it's a very simple change that is backwards compatible.

It will only work for simple on/off cases and not those where we need to pass in more complex data, but is definitely a good idea for them.

As you say, it's potentially quite a lot of work, but would be fairly easy to apply in increments - there's no need to change every Farmer CE in one PR. If you'd like to raise a PR(s) for the cases that you need (or more!), I'm sure that we'll be happy to merge them 🙂

@dburriss
Copy link
Contributor Author

@mattgallagher92 Ok nice. As I mentioned, I have a usage where I will likely need this flexibility on a couple of resources, so I will make the changes on the CEs for all boolean switches as needed (I will do all on a CE, not just the ones I need ;) )
This will mostly be compute resources e.g. WebApp, Containers, maybe VMs. It has been 4 years since I contributed to Farmer. The codebase is familiar but there are A LOT more resources and the files are A LOT bigger :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants