search

arcus-azure / arcus.scripting

Scripting with Microsoft Azure in a breeze.
https://scripting.arcus-azure.net/
MIT License
9 stars 11 forks source link

feat: Support Logic App Standard #415

Closed pim-simons closed 7 months ago

pim-simons commented 8 months ago

Add support for Logic App Standard in our Logic Apps scripts.

Closes https://github.com/arcus-azure/arcus.scripting/issues/413

netlify[bot] commented 8 months ago

Deploy Preview for arcus-scripting canceled.

Name Link
Latest commit f5f1ec2009ab20d2c6ef7eb39465921aa3c24115
Latest deploy log https://app.netlify.com/sites/arcus-scripting/deploys/655c620c1bdb4b00081ef3e1
pim-simons commented 7 months ago

I couldn't find a way to implement the prefix stuff we discussed here, since this works really a lot different in Logic App Standard. Might be worth a look later on as an enhancement though.

I split up the documentation pages to Logic App - Consumption and Logic App - Standard as I feel this makes the docs much clearer.

stijnmoreels commented 7 months ago

I couldn't find a way to implement the prefix stuff we discussed here, since this works really a lot different in Logic App Standard. Might be worth a look later on as an enhancement though.

I split up the documentation pages to Logic App - Consumption and Logic App - Standard as I feel this makes the docs much clearer.

Yes, okay, a big thx for investigating in this and good thinking in splitting this up. Should we add some redirects to the https://github.com/arcus-azure/arcus.scripting/blob/main/netlify.toml for that best?

pim-simons commented 7 months ago

I couldn't find a way to implement the prefix stuff we discussed here, since this works really a lot different in Logic App Standard. Might be worth a look later on as an enhancement though. I split up the documentation pages to Logic App - Consumption and Logic App - Standard as I feel this makes the docs much clearer.

Yes, okay, a big thx for investigating in this and good thinking in splitting this up. Should we add some redirects to the main/netlify.toml for that best?

I honestly don't really have any knowledge on that toml file, not really sure what it does 😇 I notice that Logic App isn't in the toml file currently?

stijnmoreels commented 7 months ago

I honestly don't really have any knowledge on that toml file, not really sure what it does 😇 I notice that Logic App isn't in the toml file currently?

That's right. I would believe that we need to add some [redirects] at the end that redirects the previous logic apps version to the standard or consumption now (whatever you find more appropriate and/or recommended to use).

I think these two should be added:

[[redirects]]
  from = "/:version/features/powershell/azure-logic-apps"
  to = "/:version/features/powershell/azure-logic-apps-standard"

[[redirects]]
  from = "/features/powershell/azure-logic-apps"
  to = "/features/powershell/azure-logic-apps-standard"

But I also think that we should only add them when we have a new version of the documentation released, not when they are still in /preview. So I would suggest to create a task for the next version so that we take this up when we release the next version.

pim-simons commented 7 months ago

Ah, now I see it. This parameter is also responsible if the implementation is using consumption/standard, right? Just a thought: this is a bit implicit, it might be a good idea to include Standard/Consumption in the function names? If yes: then we should make sure that we write an deprecated error when the 'old' ones, without the type, are still used.

Yes the WorkflowName parameter determines if the script executes the Consumption or Standard functionality since a workflow only exists in the Logic App Standard. It is a bit implicit, but I did not want to change the names of the scripts as that would break current implementations.

pim-simons commented 7 months ago

Yes, my concern is that this was only used for the tests, and that it could potentially be vulnerable. I'm wondering why we need it, though, as the Get-AzCachedAccessToken function already returns the same information? And once it is set, it is not used for assertion, too.

Just tried again and without the $Global:subscriptionId = '123456' the test fails with:

Cannot bind argument to parameter 'SubscriptionId' because it is an empty string.

So I honestly don't really know how else to solve this?

stijnmoreels commented 7 months ago

Yes the WorkflowName parameter determines if the script executes the Consumption or Standard functionality since a workflow only exists in the Logic App Standard. It is a bit implicit, but I did not want to change the names of the scripts as that would break current implementations.

Yes, that's why I suggested to maybe log a warning for the current module functions and create additional ones with Consumption/Standard? Can be in a follow-up PR.

stijnmoreels commented 7 months ago

Just tried again and without the $Global:subscriptionId = '123456' the test fails with:

Cannot bind argument to parameter 'SubscriptionId' because it is an empty string.

So I honestly don't really know how else to solve this?

If I read this correctly, we retrieve the $token from the Get-AzCachedAccessToken and pass that value to the next function. Why there would be a 'global' variable accessible outside the script, I don't understand directly? https://github.com/arcus-azure/arcus.scripting/pull/415/files#diff-da181b2ee5931f86b51b900f462579712ed7a306125dfccf1a2f9ebe14e730baR25

pim-simons commented 7 months ago

Integration tests fail due to https://github.com/arcus-azure/arcus.scripting/issues/416, will look into that asap.

pim-simons commented 7 months ago

Yes the WorkflowName parameter determines if the script executes the Consumption or Standard functionality since a workflow only exists in the Logic App Standard. It is a bit implicit, but I did not want to change the names of the scripts as that would break current implementations.

Yes, that's why I suggested to maybe log a warning for the current module functions and create additional ones with Consumption/Standard? Can be in a follow-up PR.

Lets do that in a follow up indeed, than we have a bit more time to look at this 👍🏻

pim-simons commented 7 months ago

If I read this correctly, we retrieve the $token from the Get-AzCachedAccessToken and pass that value to the next function. Why there would be a 'global' variable accessible outside the script, I don't understand directly?

I don't really know the background of this script.

What I think happens is that because we Mock the Get-AzCachedAccessToken the global variables are not set. That is why the $Global:subscriptionId = '123456' is needed. Does that make sense?

pim-simons commented 7 months ago

If I read this correctly, we retrieve the $token from the Get-AzCachedAccessToken and pass that value to the next function. Why there would be a 'global' variable accessible outside the script, I don't understand directly?

I don't really know the background of this script.

What I think happens is that because we Mock the Get-AzCachedAccessToken the global variables are not set. That is why the $Global:subscriptionId = '123456' is needed. Does that make sense?

As discussed, I removed the global variables from the scripts where they were not already present.