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

Implementing paths #6181

Merged
merged 7 commits into from Nov 29, 2019

Conversation

@FriedrichWeinmann
Copy link
Contributor

FriedrichWeinmann commented Nov 5, 2019

Type of Change

  • Bug fix (non-breaking change, fixes # )
  • New feature (non-breaking change, adds functionality, fixes # )
  • Breaking change (effects multiple commands or functionality, fixes # )
  • Ran manual Pester test and has passed (`.\tests\manual.pester.ps1)
  • Adding code coverage to existing functionality
  • Pester test is included
  • If new file reference added for test, has is been added to github.com/sqlcollaborative/appveyor-lab ?
  • Nunit test is included
  • Documentation
  • Build system

Purpose

These new commands introduce better path resolution for common paths usually covered under environment variables ($env:temp, $env:AppData, $env:LocalAppdata, $env:ProgramData). They should find viable paths cross-platforms.

They also allow redirecting those paths at the user's convenience. For example when the default paths are unsuitable due to permissions reasons.

At the same time, they are a foundation to be able to easily create more, named paths utilizing the same tools.

Notes

  • All internal use was corrected / switched over.
  • Take care not to use this in scriptblocks that are invoked remotely, as the remote system might not have dbatools
Friedrich Weinmann Friedrich Weinmann
@FriedrichWeinmann FriedrichWeinmann requested a review from potatoqualitee Nov 5, 2019
@wsmelton

This comment has been minimized.

Copy link
Member

wsmelton commented Nov 5, 2019

This seems to overcomplicate things to me. Why would we not use environment variables that are in PowerShell? These all work the same on any OS?

@FriedrichWeinmann

This comment has been minimized.

Copy link
Contributor Author

FriedrichWeinmann commented Nov 5, 2019

The paths should work on all relevant OSes.
The environment variables will be picked up if available, but no longer solely dependent on that.
However this implementation also gives the user the ability to explicitly redirect it using the configuration system (while offering a user-friendly front-end).
Chrissy requested this due to frequent issues when running dbatools from the SQL Server Agent.

@wsmelton

This comment has been minimized.

Copy link
Member

wsmelton commented Nov 5, 2019

This needs to be added to the list of things to document for contributors if we implement it. Otherwise we will see folks just use env on the code changes.

@FriedrichWeinmann

This comment has been minimized.

Copy link
Contributor Author

FriedrichWeinmann commented Nov 5, 2019

Probably even better to include tests to catch "unauthorized" $env: usage.
Which I'll throw in this afternoon to make the tests happy (see currently failed tests). 0:30am, so not updating this right now anymore :)

@potatoqualitee

This comment has been minimized.

Copy link
Member

potatoqualitee commented Nov 5, 2019

agreed, that'd be great. one thing tho is that we have to ensure that we can still use $env within a Scriptblock in Invoke-Command/2 since the dbatools command will not be available remotely.

@wsmelton

This comment has been minimized.

Copy link
Member

wsmelton commented Nov 6, 2019

Need to add Save-DbaDiagnosticQueryScript to this PR if you can Fred. It uses the MyDocuments folder path.

@potatoqualitee

This comment has been minimized.

Copy link
Member

potatoqualitee commented Nov 27, 2019

heck yah, these are failures i can fix 👍

@potatoqualitee

This comment has been minimized.

Copy link
Member

potatoqualitee commented Nov 29, 2019

fake failure 👍 merging, thanks, fred!

@potatoqualitee potatoqualitee merged commit 1f4f89f into development Nov 29, 2019
4 of 5 checks passed
4 of 5 checks passed
Module imports on all platforms (ubuntu-latest)
Details
Module imports on all platforms (windows-latest)
Details
Module imports on all platforms (macOS-latest)
Details
continuous-integration/appveyor/branch AppVeyor build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@potatoqualitee potatoqualitee deleted the PathUpdates branch Nov 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.