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

Logic App Workflow and Integration Account Snippet #3919

Merged
merged 5 commits into from
Aug 21, 2021

Conversation

rajyraman
Copy link
Contributor

Snippet for Logic App workflow was creating Integration Account, and there was no snippet for Integration Account.

  • I have a snippet that is either a single, generic resource or multi resource that uses parent-child syntax
  • I have checked that there is not an equivalent snippet already submitted
  • I have used camelCasing unless I have a justification to use another casing style
  • I have placeholders values that correspond to their property names (e.g. dnsPrefix: 'dnsPrefix'), unless it's a property that MUST be changed or parameterized in order to deploy. In that case, I use 'REQUIRED' e.g. keyData
  • I have my symbolic name as the first tab stop ($1) in the snippet. e.g. res-aks-cluster.bicep
  • I have a resource name property equal to "name"

@miqm
Copy link
Collaborator

miqm commented Aug 7, 2021

A suggestion - perhaps we could include in the snippet json(loadTextContent('{placeholder}')) for loading LogicApp definition from a file?

@rajyraman
Copy link
Contributor Author

@miqm - I thought this is a simple fix, but I cannot get the tests working.

  1. Complains about missing file
  2. main.combined.bicep is always generated incorrectly

I am not sure what I am doing wrong.
image
image
image

@miqm
Copy link
Collaborator

miqm commented Aug 9, 2021

@rajyraman It's not you, seems like snippet tests runner is trying to load the file while it should ignore it. It needs to be changes/fixed, I'll create issue for it and see what can be done.

@rajyraman
Copy link
Contributor Author

rajyraman commented Aug 9, 2021

@miqm - I can't believe that I could not see the missing '} 🤦. I am now getting this error.
image

Thank you fixing the underlying issue. I will test it with your PR.

@miqm
Copy link
Collaborator

miqm commented Aug 12, 2021

Hey @rajyraman, both issues you've been experiencing are fixed, please rebase and try now.

name: /*${2:'name'}*/'name'
location: resourceGroup().location
properties: {
definition: json(loadTextContent(/*${3:'REQUIRED'}*/'REQUIRED')).definition
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead required put logicApp.json it has to be nam of the file with definition. In addition, remove the .definition from the end and from json file just leave object starting from schema.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@miqm - I can change the name, but CONTRIBUTING.md, says that I should use 'REQUIRED' if that property must be changed before deployment. All Logic Apps JSON's have definition property, and so I am mapping property to this definition property.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing is that there are 2 tests. One that uses mapping done in main.bicep and second that just compiles the snippet. Your code is failing with the second one as there is no REQUIRED file to be loaded.
@anthony-c-martin should we make an exemption from the REQUIRED rule and allow for parameter that needs to be changed different file name?

@anthony-c-martin
Copy link
Member

@rajyraman - would you mind enabling "Allow edits from maintainers" for this PR, and I'll push the fixes for your tests.

@rajyraman
Copy link
Contributor Author

@anthony-c-martin - It seems to be already enabled.
image

Copy link
Member

@anthony-c-martin anthony-c-martin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for contributing!

@anthony-c-martin anthony-c-martin merged commit 9a8cf3a into Azure:main Aug 21, 2021
@miqm miqm mentioned this pull request Sep 1, 2021
6 tasks
This pull request was closed.
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.

3 participants