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

Enable discovering modules that have names same as a culture (e.g. Az) #8777

Merged
merged 6 commits into from Feb 20, 2019

Conversation

@SteveL-MSFT
Copy link
Member

SteveL-MSFT commented Jan 29, 2019

PR Summary

The suggested fix by @lzybkr is to not do this additional culture name check for the first set of folders (which should be the module name). This PR fixes a few things:

  • Add logic to skip checking for possible resource directories for the first set of sub directories from the top level.
  • There was an additional skip if the folder is hidden, rather than doing an explicit attribute check, change the EnumerationOption to skip hidden folders.
  • Since the IsPossibleModuleDirectory() helper now only checks to see if the name matches a culture, renamed to IsPossibleResourceDirectory()
  • When getting the default modules, we don't search recursively into individual module folders, so removed additional check for possible resource directory.

PR Context

The current code always checks if a potential module folder is named after a culture and assumes it contains resources files as an optimization to avoid looking in that folder. This results in the new Az module not being found as Az is also the name of a culture.

Fix #8125

PR Checklist

Copy link
Member

rjmholt left a comment

LGTM

@SteveL-MSFT SteveL-MSFT force-pushed the SteveL-MSFT:az-module-discovery branch from 4314122 to 3ed7768 Jan 29, 2019
Copy link
Member

daxian-dbw left a comment

LGTM

@SteveL-MSFT

This comment has been minimized.

Copy link
Member Author

SteveL-MSFT commented Feb 20, 2019

The static analysis failure is not related to this PR. I'll submit a fix for it in a separate PR.

@daxian-dbw daxian-dbw merged commit b06ad6a into PowerShell:master Feb 20, 2019
7 of 9 checks passed
7 of 9 checks passed
PowerShell-CI-static-analysis #PR-8777-20190220.01 failed
Details
Codacy/PR Quality Review Hang in there, Codacy is reviewing your Pull request.
Details
CodeFactor 6 issues fixed. 5 issues found.
Details
PowerShell-CI-install-ps #PR-8777-20190220.01 succeeded
Details
PowerShell-CI-linux #PR-8777-20190220.01 succeeded
Details
PowerShell-CI-macos #PR-8777-20190220.01 succeeded
Details
PowerShell-CI-windows #PR-8777-20190220.01 succeeded
Details
WIP Ready for review
Details
license/cla All CLA requirements met.
Details
@daxian-dbw

This comment has been minimized.

Copy link
Member

daxian-dbw commented Feb 20, 2019

The PR description is updated as we are not using recursion when searching for modules.

@SteveL-MSFT SteveL-MSFT deleted the SteveL-MSFT:az-module-discovery branch Feb 20, 2019
@ronhowe

This comment has been minimized.

Copy link

ronhowe commented Feb 28, 2019

Could someone help me understand the delivery vehicle and timing for this fix? Is this a fix for Windows PowerShell 5.1 or for PowerShell Core (6.0)? If Windows PowerShell 5.1, would that mean delivery would be part of a 5.2 type release? Are consumers today completely blocked from building module dependencies on the new Az module until this is resolved?

@iSazonov

This comment has been minimized.

Copy link
Collaborator

iSazonov commented Feb 28, 2019

@ronhowe The fix is only for PowerShell Core. You'll get it in PowerShell Core 6.2 RC in hours and in release PowerShell Core 6.2 in weeks.

Windows PowerShell 5.2 is not expected. Although MSFT may port this fix to 5.1.

Are consumers today completely blocked from building module dependencies on the new Az module until this is resolved?

The problem is only for auto discovering. You could use full path to explicitly load the module.

@joeyaiello

This comment has been minimized.

Copy link
Member

joeyaiello commented Feb 28, 2019

@ronhowe: to reiterate what @iSazonov said, our bar for servicing Windows PowerShell is very high now, and we do not plan to do a 5.2 release. This has resulted in a small handful of security and accessibility fixes, and some bug fixes to fix regressions associated with those two.

That being said, I want to talk to the Azure PowerShell team about this (/cc @sphibbs and @markcowl for context), as I don't want us limiting the adoption of Az in any way.

In the meantime, using Import-Module Az explicitly (even without the full path, I believe), the module will load and you can use it.

Apparently, it may also be the case that RequiredModules will fail because it secretly uses Get-Module -ListAvailable under the hood (which is where we're doing the locale optimization).

But like I said, I'll follow up with the Azure PS to get their sense of the impact.

Pure curiosity and off-topic, but I'd also love to hear what's keeping you from using PowerShell Core. :)

@joeyaiello

This comment has been minimized.

Copy link
Member

joeyaiello commented Feb 28, 2019

Dove a little deeper on this...@ronhowe: what exactly is the problem you're experiencing?

We've confirmed that RequiredModules doesn't work with Az, but Az is a phony module that exports nothing, and you shouldn't be pulling every single Az.* module into your RequiredModules, you should be explicitly specifying the Az.* modules that you need.

Additionally, because Az exports nothing and is the only module affected here, autoloading shouldn't actually be impacted for any of the cmdlets in Az.* modules.

So again, how are you actually being affected here?

@ronhowe

This comment has been minimized.

Copy link

ronhowe commented Feb 28, 2019

Dove a little deeper on this...@ronhowe: what exactly is the problem you're experiencing?

We've confirmed that RequiredModules doesn't work with Az, but Az is a phony module that exports nothing, and you shouldn't be pulling every single Az.* module into your RequiredModules, you should be explicitly specifying the Az.* modules that you need.

Additionally, because Az exports nothing and is the only module affected here, autoloading shouldn't actually be impacted for any of the cmdlets in Az.* modules.

So again, how are you actually being affected here?

I just read this and that's the exact problem. I can try to be more selective about what my module imports.

@ronhowe

This comment has been minimized.

Copy link

ronhowe commented Feb 28, 2019

As to why we aren't using PowerShell Core? I'm all for that. It's a bit more difficult to spearhead that adoption across my company. I am at the frontline of that effort, but it's a much slower ship and we have more immediate needs.

RDIL pushed a commit to RDIL/PowerShell that referenced this pull request Mar 13, 2019
PowerShell#8777)

Here are the major changes:
- Add logic to skip checking for possible resource directories for the first set of sub directories from the top level.
- There was an additional skip if the folder is hidden, rather than doing an explicit attribute check, change the `EnumerationOption` to skip hidden folders.
- Since the `IsPossibleModuleDirectory()` helper now only checks to see if the name matches a culture, renamed to `IsPossibleResourceDirectory()`
- When getting the default modules, we don't search recursively into individual module folders, so removed additional check for possible resource directory.
@KevinMarquette

This comment has been minimized.

Copy link
Contributor

KevinMarquette commented Apr 1, 2019

The impact is that the user is given the AZ as an easy way to instal and manage the Azure module with all it's dependent modules. But the user cannot call Get-Module to see what version of the module they have.

With everyone writing their own module install\update logic, we have to walk the $env:psmodulepath just for this module to determine if we should install the module or update it. So we are leaving it on the end user to solve for this.

@vexx32

This comment has been minimized.

Copy link
Collaborator

vexx32 commented Apr 1, 2019

You'd think that the Azure team would be verifying that the module works as expected in a majority of use cases before finalizing a module structure that is actually broken on both the supposedly supported platforms.

I guess for whatever reason... these cases were overlooked. I don't know that I could fault the PS team for this behaviour moreso than the Azure team for not checking that the module functions as expected with the structure they chose. :/

@lukeb1961

This comment has been minimized.

Copy link

lukeb1961 commented Oct 14, 2019

Both teams are failing ME - the user.
by not fixing in 5.x and by the AZ name not being tested properly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.