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

Support for "includeFile()" like functionality #471

Closed
alex-frankel opened this issue Sep 9, 2020 · 49 comments · Fixed by #2501
Closed

Support for "includeFile()" like functionality #471

alex-frankel opened this issue Sep 9, 2020 · 49 comments · Fixed by #2501
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@alex-frankel
Copy link
Collaborator

Is your feature request related to a problem? Please describe.
Should read a file and return as a string to make inline-ing scriptContent to the deploymentScripts resource (and other equivalent scenarios like APIM) easier.

Requested in #439 and by @tyconsulting

@alex-frankel alex-frankel added the enhancement New feature or request label Sep 9, 2020
@ghost ghost added the Needs: Triage 🔍 label Sep 9, 2020
@kilasuit
Copy link

kilasuit commented Sep 9, 2020

will this include from file in local repo (e.g. on the disk next/near to the biceps) or will that need to be ARM accessible (i.e. via the web)

@alex-frankel
Copy link
Collaborator Author

it would be local

@alex-frankel
Copy link
Collaborator Author

we actually should probably support both

@kilasuit
Copy link

kilasuit commented Sep 9, 2020

Both would be ideal, but supporting local is a real need for this and other parts too

@marcre
Copy link
Collaborator

marcre commented Sep 10, 2020

This makes a lot of sense for embedding values that need to be given to RPs. The one caveat I'd throw in is lets keep it specific to values and not allow embedded bicep code. I don't think we want to bring back the full c pre-processor.

@miqm
Copy link
Collaborator

miqm commented Jan 14, 2021

This makes a lot of sense for embedding values that need to be given to RPs. The one caveat I'd throw in is lets keep it specific to values and not allow embedded bicep code. I don't think we want to bring back the full c pre-processor.

Why not? It might be useful to have include or ifdef directives i. e. if we have some common bicep file with some standard parameters that we want for our modules (i.e. project name, env name) or want to differ our generated template for "debug" or "release" modes.

@skyaddict
Copy link

I would like to see a feature like this to simplify deploying a logic app. After making changes in our test environment I would like to pull the logic app "code" and insert it into my logic app bicep file. This way i don't have to do all the escaping manually. If there is already a way to manage logic apps please let me know.

In my scenario I have several logic apps that all use the same storage account and api connections. I am creating theses in a bicep file in order to include the logic app I need to download the logic app arm template remove the stuff I don't need created clean it up and decompile it to bicep so the escaping is handled correctly. I would like to see this greatly simplified.

@miqm
Copy link
Collaborator

miqm commented Mar 17, 2021

@alex-frankel - as we talked I'll take this one and see what can be done.

As we talked, we will start with loading file contents to a string. So my first question is about the function name - I feel that includeFile() can be confusing, as it might suggest including part of bicep code instead file content.

I'd go with readContent()/readFile() or loadContent()/loadFile() - but final decision I'd leave to you :)

Second thing - more technical one (probably to @anthony-c-martin or @majastrz) - as I understand, we will include the body of the the file content as json string, adding necessary escaping and line breaks; similar to multiline string behaviour?

@alex-frankel
Copy link
Collaborator Author

Good point - I like loadFile(), but curious what others think

@skyaddict
Copy link

I kind of like loadContent since that is what is happening. LoadFile sounds like an attachment. What about insertContent?

I am curious how you envision passing parameters? Would variable scope apply to the loaded content?

@marcre
Copy link
Collaborator

marcre commented Mar 17, 2021

I like having content in the name. Makes it more clear that not a bicep file you are including.

@johndowns
Copy link
Contributor

Will this function ever support binary files (e.g. convert them to base64-encoded strings, like to handle certificate .pfx files)? If not I wonder if it's best to name it something like loadStringContent() or loadTextContent().

@marcre
Copy link
Collaborator

marcre commented Mar 17, 2021

Interesting question. I think to do binary content you'd need a different function or argument to indicate how we should encode the file.

@alex-frankel
Copy link
Collaborator Author

It'd be nice if we have one function with different arguments IMO. Maybe by default with one argument we assume it's going to be loaded as a string, but if there are two arguments we could support different formats.

@alex-frankel
Copy link
Collaborator Author

I am curious how you envision passing parameters? Would variable scope apply to the loaded content?

Can you clarify what you mean by this? This would load the content as a string that you would pass directly to a resource that supports a relevant string property. The canonical use case being something like deployment scripts:

resource ds 'Microsoft.Resources/deploymentScripts@2020-10-01' = {
  kind: 'AzurePowerShell'
  name: 'sampleDs'
  location: 'eastus'
  properties: {
    azPowerShellVersion: '5.6.0'
    scriptContent: loadContent('./my-script.ps1')
    ...
  }
}

@miqm
Copy link
Collaborator

miqm commented May 31, 2021

@alex-frankel @majastrz @anthony-c-martin A question - should we support following syntaxes?

var fileName = 'file.sh'
var script = loadTextContent(fileName)
var files = [
 { 
  name: 'file.sh'
  encoding: 'us-ascii'
 }
]
var script = loadTextContent(files[0].name, files[0].encoding)

@majastrz
Copy link
Member

majastrz commented Jun 1, 2021

In the first iteration of this feature, I don't think we need to. We should enforce that the arguments to that function are compile-time constants, though.

@miqm
Copy link
Collaborator

miqm commented Jun 1, 2021

In the first iteration of this feature, I don't think we need to. We should enforce that the arguments to that function are compile-time constants, though.

Hmm, I managed to implement it yesterday :) I'm taking argument type and checking if it's a StringLiteralType and then obtaining RawValue from it.

@mauve
Copy link

mauve commented Jun 22, 2021

It would be really cool if we could support simple replacement during loading, example:

cloud-init.txt

# cloud-init
users:
  - default
  - name: {{ username }}
    groups: sudo {{ groups }}
    shell: {{ shell }}

vm.bicep

var values = {
  username: 'mikael'
  groups: ''
  shell: '/bin/bash'
}

resource my_vm 'Microsoft.Compute/virtualMachine' = {
  properties: {
    osProfile: {
      customData: loadContentAsBase64('user-data.template.yml', values)

      // or:

      customData: base64(loadTemplate('user-data.template.yml', values))
    }
  }
}

@alex-frankel
Copy link
Collaborator Author

You should be able to use the format() function to do some string replacement. Not quite as elegant as having named variables, but curious how far this would get you.

# cloud-init
users:
  - default
  - name: {0}
    groups: sudo {1}
    shell: {2}
var values = {
  username: 'mikael'
  groups: ''
  shell: '/bin/bash'
}

var cloudInit = loadContentAsBase64('user-data.template.yml')

resource my_vm 'Microsoft.Compute/virtualMachine' = {
  properties: {
    osProfile: {
      customData: format(cloudInit, values[0], values[1], values[2])
    }
  }
}

@onionhammer
Copy link

onionhammer commented Jun 22, 2021

@alex-frankel what's the reasoning for not supporting variable names in string replacement? Is it an issue of scope or level of effort (vs level of usefulness)? Is it technically not feasible?

@alex-frankel
Copy link
Collaborator Author

It's probably feasible, but is not supported today, so we'd need to justify it/prioritize it. The above sample should work as soon as Miq's PR is checked in.

@miqm
Copy link
Collaborator

miqm commented Jun 22, 2021

you can use replace function in the runtime. Remember, that loading occurs during compilation time, not deployment. more often you would replace based on some parameters rather than baking various versions of same file.

Another thing you need to remember - ARM template (the output of bicep) is limited to 4MB of size. loading with substitution would generate lots of IL code.

Plus, values for substitution would need to be known during compile time - so they would need to be static.

I'd suggest you go with format or replace functions.

@miqm
Copy link
Collaborator

miqm commented Jun 30, 2021

🎉

@IPJT
Copy link

IPJT commented Aug 31, 2021

Hi there,
Is it possible to do this in JSON ARM templates rather than bicep ARM templates?
Thank you!

@miqm
Copy link
Collaborator

miqm commented Aug 31, 2021

Hi there,
Is it possible to do this in JSON ARM templates rather than bicep ARM templates?
Thank you!

No, it's bicep functionality only.

@IPJT
Copy link

IPJT commented Sep 1, 2021

Hi there,
Is it possible to do this in JSON ARM templates rather than bicep ARM templates?
Thank you!

No, it's bicep functionality only.

That's a pity. Thank you for your swift reply!

@slavizh
Copy link
Contributor

slavizh commented Sep 1, 2021

@IPJT any reason not to move to bicep? After all the end result of bicep code is deployment via ARM template.

@IPJT
Copy link

IPJT commented Sep 1, 2021

@IPJT any reason not to move to bicep? After all the end result of bicep code is deployment via ARM template.

@slavizh
Well, realizing you can do everything with Bicep + additional things that you can with JSON ARM templates it seems like it would have been the better decision. However, we already have quite a lot of our infra described in JSON that would take some time to move to Bicep. Additionally, the developers within the project who will be responsible to update the templates as they develop features requiring modified/additional infra has already started to learn JSON ARM.

Maybe a classic "better now than never" though.. 😄

@slavizh
Copy link
Contributor

slavizh commented Sep 1, 2021

@IPJT good reasons. We are in this process as well. We have started by moving different templates to bicep so we can spot any issues/missing features. We plan to continue convert our templates to bicep over the next year at least. One of the latest interesting feature about bicep is that you can use ARM template as modules and not only bicep files. This may help in cases where you can move some parts of the code to bicep but others are intact. Overall my advise is to take some general decisions to move forward as definitely there could be more features only available in Bicep. With the decisions also make a plan on how to move to Bicep. You can take approach of move to Bicep where it is really necessary and do things in stages to avoid rushing and converting in short time.

@ghost ghost locked as resolved and limited conversation to collaborators May 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.