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

Make a nicer deployment API #452

Closed
wants to merge 9 commits into from
Closed

Make a nicer deployment API #452

wants to merge 9 commits into from

Conversation

isaacabraham
Copy link
Member

This PR closes #451.

The changes in this PR are as follows:

  • Add extension methods onto the deployment type itself for things like Deploy and Write.
  • Take advantage of optional arguments and overloads to make an easier API to consume.
  • Mark old functions as Obsolete with the intention of removing them for V2.
template |> quickWrite "template" // becomes...
template.ToFile "template"

template |> execute "functions-rg" Deploy.NoParameters // becomes...
template.Deploy "functions-rg"

template |> execute "functions-rg" [ "param1", "value1" ] // becomes...
template.Deploy ("functions-rg", [ "param1", "value1" ])

Deploy.setSubscription Guid.Empty
template |> execute "functions-rg" [ "param1", "value1" ] // becomes...

template.Deploy ("functions-rg", [ "param1", "value1" ], Guid.Empty)

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.

@isaacabraham
Copy link
Member Author

@viktorvan I know you were interested in the subscription stuff - this would probably help. I'm even thinking of putting in more checks like if you have multiple subscriptions, forcing you to supply one.

@isaacabraham isaacabraham added this to the v2 milestone Nov 8, 2020
@viktorvan
Copy link
Contributor

The way I imagine working with subscriptionIds would be either providing it as a string on the command line, or reading it as a string from an environment variable.

Maybe it would be useful with a SubscriptionId type with a parse function.

type SubscriptionId = SubscriptionId of Guid
module SubscriptionId =
  let parse str = Guid.Parse(str) |> SubscriptionId

but with some error checks and friendlier error messages probably :)

If it is kept as a Guid I still would need to parse it from my input string to a Guid, so it might as well be a “real” type I guess?

The Subscription Guid is created by azure, right, so I don’t think you’d ever would write code like
let mySubscription = Guid.NewGuid() so I don’t really care that the underlying type is a Guid, it’s just an identifier.

@isaacabraham
Copy link
Member Author

The way I imagine working with subscriptionIds would be either providing it as a string on the command line, or reading it as a string from an environment variable.

Doing so would be done outside of Farmer itself I think - I really don't want to have some "magic" environment variable that Farmer reads (maybe this could be a wrapper designed to make Farmer work OOTB for e.g. CI/CD processes...).

The Subscription Guid is created by azure, right, so I don’t think you’d ever would write code like
let mySubscription = Guid.NewGuid() so I don’t really care that the underlying type is a Guid, it’s just an identifier.

Making it a GUID rather than a string at least lets us confirm if the string supplied is a valid GUID - maybe someone uses an environment variable and mistypes the environment variable key or similar.

@viktorvan
Copy link
Contributor

Doing so would be done outside of Farmer itself I think - I really don't want to have some "magic" environment variable that Farmer reads (maybe this could be a wrapper designed to make Farmer work OOTB for e.g. CI/CD processes...).

Yes, I do not suggest using any magic, the point I was trying to make was what it would be like to use this feature:

In my farmer-project I would get the subscriptionId from my configuration (in one way or another), for example

let mySubscriptionId = 
    System.Environment.GetEnvironmentVariable "MY_SUBSCRIPTION_KEY" 
    |> Guid.Parse

let template = ...

template.Deploy ("my-resource-group", [], mySubscriptionId)

I would have to parse my string to Guid. And in that case I think it would make sense to have a SubscriptionId type so I instead can do:

let mySubscriptionId = 
    System.Environment.GetEnvironmentVariable "MY_SUBSCRIPTION_KEY" 
    |> SubscriptionId.parse

But maybe it's overkill.

Making it a GUID rather than a string at least lets us confirm if the string supplied is a valid GUID - maybe someone uses an environment variable and mistypes the environment variable key or similar.

Yes, I agree that it should at least not be a plain string :)

# Conflicts:
#	src/Farmer/Deploy.fs
#	src/Farmer/Writer.fs
# Conflicts:
#	samples/scripts/deployment-script.fsx
#	src/Farmer/Deploy.fs
#	src/Tests/Template.fs
@isaacabraham
Copy link
Member Author

This PR will never get merged, but it is useful to have as an example of what a new API could look like.

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.

Revisit Deployment API
2 participants