Azure / azure-sdk-tools

Tools repository leveraged by the Azure SDK team.
MIT License
113 stars 177 forks source link

Use common.ps1 in New-TestResources to detect the language. #1082

Open pakrym opened 4 years ago

pakrym commented 4 years ago

https://github.com/Azure/azure-sdk-tools/pull/1077#discussion_r501904126

@chidozieononiwu is working on some common scripts one of which would give you the language. If you import common.ps1 it will import the language-settings.ps1 which will have $language defined.

heaths commented 4 years ago

Rather than detecting the language, how about a common module to save secrets? We can use that as-is, and as we add support for more languages/platforms, we don't need to update the New-TestResources scripts.

Also, I recommend making imported scripts modules instead (e.g. .psm1 files). They get scoped so there's always a way to avoid collisions. There are other advantages as well. Run help about_Advanced_functions in PowerShell for more details.

pakrym commented 4 years ago

I agree with @heaths here, we should add support for environment file to other languages and OSes instead.

chidozieononiwu commented 4 years ago

Rather than detecting the language, how about a common module to save secrets? We can use that as-is, and as we add support for more languages/platforms, we don't need to update the New-TestResources scripts.

Also, I recommend making imported scripts modules instead (e.g. .psm1 files). They get scoped so there's always a way to avoid collisions. There are other advantages as well. Run help about_Advanced_functions in PowerShell for more details.

We actually ran into some issues with modules earlier, which is why we decided to switch back to scripts which seems simpler to implement. @weshaggard

weshaggard commented 4 years ago

I agree with @heaths that if we can we should avoid having to know anything about a given language in our common scripts, it is in fact why we are creating the common.ps1/language-settings.ps1 scripts so we can have only common code in eng/common, and any language specific changes can be in the language repos. So I'm definitely Ok with having a common way do this and allow languages to opt-out or override the default behavior as needed.

As for modules as @chidozieononiwu pointed out they have been mostly nothing but trouble in our usages and we have reverted back. Powershell modules have also recently been the root cause of a build issue in our releases yesterday, in fact it was the installing of the Az modules for our test provisioning that took out most of our test pipelines yesterday because of PS Gallery issues. Even ignoring the PS gallery issues modules seem to have other issues and as best I can tell provide little to no benefit over us using simple ps1 scripts, so we switched back to scripts for our stuff.

heaths commented 4 years ago

That's from installing modules via powershellgallery.com. PowerShell modules don't have to go there. They are just .psm1 files - at a minimum - that you can write like .ps1 files. The difference is that you import them via path:

import-module -path ../scripts/common.psm1

Modules not only inherit scope, but can have their own scope. Want private variables between scripts? You can do that with modules. You can't with dot-sourced scripts. Lots of other benefits that have nothing to do with powershellgallery.com (or other repos).

weshaggard commented 4 years ago

PSGallery is definitely one big issue but even importing via path has its own problems especially while developing, just the process of install/uninstall all the time to test out changes is a big enough pain to not use them for these purposes. I'm don't think the private variables encapsulation is very interesting for our usages, in fact a number of our usages we want to share the variable scope. At this point I'd like for us to stick with script files and if we run across issues that we believe could be solved with modules we can re-evaluate in the future.

heaths commented 4 years ago

Script files for a single entry point are nice, yes. I agree. But the existing scripts you reference seem to require dot-sourcing, which makes them even harder to use. You can remove what they add without being explicit (e.g. del function:\do-foo). If you're going to use scripts, they should be a single entry point. Sub-functions are fine. But dot-sourcing scripts to pull in common features is problematic. Script modules were designed to fix all those problems.

weshaggard commented 4 years ago

Our current plan is to only every dot source in the common.ps1 script from any entry point script and then they would have access to all the support of our shared scripts.