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

add machine scope #2039

Merged
merged 3 commits into from
Jul 17, 2019
Merged

Conversation

TylerLeonhardt
Copy link
Member

PR Summary

Fixes #2024

Per the vscode team's request, we now need to mark executables with a scope: "machine" to aid in remote development scenarios.

@sandy081 is there a recommended way to test this change? I couldn't use Remote - Container because it just gives me the latest release of the PowerShell extension.

PR Checklist

Note: Tick the boxes below that apply to this pull request by putting an x between the square brackets.
Please mark anything not applicable to this PR NA.

  • PR has a meaningful title
  • Summarized changes
  • N/A PR has tests
  • This PR is ready to merge and is not work in progress
    • If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready

@sandy081
Copy link

As mentioned, machine scope settings has two characteristics

  • They are not overridable at workspace/folder level
  • In Remote Set up, User value is ignored.

@TylerLeonhardt
Copy link
Member Author

They are not overridable at workspace/folder level

This is what the IsExecutable property does today IIRC

In Remote Set up, User value is ignored.

Yep perfect for exe paths.

I guess I've done this right, but how do I test it? As in... How can I test a local build of an extension in a remote setup?

package.json Outdated
@@ -755,6 +757,7 @@
"powershell.developer.powerShellExePath": {
"type": "string",
"default": "",
"scope": "machine",
"isExecutable": true,
"description": "Deprecated. Please use the 'powershell.powerShellExePath' setting instead"
Copy link
Contributor

@rkeithhill rkeithhill Jun 21, 2019

Choose a reason for hiding this comment

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

Maybe we should get rid of this deprecated setting? It has been deprecated for a long time.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@TylerLeonhardt
Copy link
Member Author

TylerLeonhardt commented Jun 25, 2019

well it seems this PR broke our CI... so it did something... I'll switch the test to set a different setting...

That makes me wonder... @sandy081 was "isExecutable": true deprecated at some point? That was originally used to meet the first bullet point:

  • They are not overridable at workspace/folder level

@sandy081
Copy link

@TylerLeonhardt
Copy link
Member Author

Ah. Missed that point, thanks! I'll update this PR to remove that.

@TylerLeonhardt TylerLeonhardt merged commit ddbf324 into PowerShell:master Jul 17, 2019
@TylerLeonhardt TylerLeonhardt deleted the add-machine-scope branch July 17, 2019 22:16
rjmholt pushed a commit to rjmholt/vscode-powershell that referenced this pull request Jul 24, 2019
* add machine scope

* use a different setting for test and add user setting test

* remove isExecutable and remove powershell.developer.powerShellExePath
rjmholt added a commit that referenced this pull request Jul 26, 2019
* Edit snippets to support $TM_SELECTED_TEXT (#1945)

Edit all-and-only applicable snippets to support $TM_SELECTED_TEXT,
where "applicable" is approximated by whether a snippet contains a
user-specified PowerShell expression, block, or body. Do not add,
remove, or otherwise change any placeholder number or name in order to
preserve backwards-compatibility.

Edit the following snippets (listed by name, not prefix):
- Class
- Constructor
- Method
- Enum
- Cmdlet
- Function-Advanced
- DSC Resource Provider (class-based)
- DSC Resource Provider (function-based)
- comment block
- do-until
- do-while
- while
- for
- for-reversed
- foreach
- function
- Function-Inline
- if
- elseif
- else
- switch
- try-catch
- try-catch-finally
- try-finally
- Workflow
- Workflow ForEachParallel
- Workflow InlineScript
- Workflow Parallel
- Workflow Sequence
- Region Block
- IfShouldProcess
- CalculatedProperty
- PesterDescribeContextIt
- PesterDescribeBlock
- PesterContextIt
- PesterContext
- PesterIt

* Add ArgumentCompleter snippets (#1946)

* Define snippet named 'ArgumentCompleterAttribute with ScriptBlock'

* Define snippet named 'IArgumentCompleter Class'

* Define snippet named 'ArgumentCompleterAttribute ScriptBlock'

* Add #Requires snippets (#1974)

* Add script requirement directive snippets

Adds the following snippets (listed by name, not prefix):
- Requires Assembly
- Requires Assembly Path
- Requires Assembly Version
- Requires Module
- Requires Module RequiredVersion
- Requires Module Version
- Requires PSEdition
- Requires PSSnapin
- Requires PSSnapin Version
- Requires RunAsAdministrator
- Requires ShellId
- Requires Version

* Fix node version detect logic to handle node v10 (#2025)

* #1019: Get format settings from document editor instead of global. (#2035)

* Update PSSA docs Url to point to master branch because master is now the default branch (#2037)

* add machine scope (#2039)

* add machine scope

* use a different setting for test and add user setting test

* remove isExecutable and remove powershell.developer.powerShellExePath

* Add param-block snippet (#2081)
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.

Flag machine specific configurations like paths as machine scoped
4 participants