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

Deprecate PythonPath - Part 1 #11011

Merged
merged 7 commits into from
Apr 8, 2020
Merged

Deprecate PythonPath - Part 1 #11011

merged 7 commits into from
Apr 8, 2020

Conversation

karrtikr
Copy link

@karrtikr karrtikr commented Apr 7, 2020

For #11015

All of this has already been reviewed. Just have a quick glance making sure all security changes are behind an experiment, and this should be good to merge.

  • Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR).
  • Title summarizes what is changing.
  • Has a news entry file (remember to thank yourself!).
  • Appropriate comments and documentation strings in the code.
  • Has sufficient logging.
  • Has telemetry for enhancements.
  • Unit tests & system/integration tests are added/updated.
  • Test plan is updated as appropriate.
  • package-lock.json has been regenerated by running npm install (if dependencies have changed).
  • The wiki is updated with any design decisions/details.

@karrtikr karrtikr marked this pull request as ready for review April 7, 2020 17:59
@karrtikr karrtikr changed the title Deprecate PythonPath - Part 1 WIP - Deprecate PythonPath - Part 1 Apr 7, 2020
@karrtikr karrtikr force-pushed the pythonPath branch 2 times, most recently from d16439e to 810191f Compare April 7, 2020 18:47
@karrtikr karrtikr changed the title WIP - Deprecate PythonPath - Part 1 Deprecate PythonPath - Part 1 Apr 7, 2020
@codecov-io
Copy link

codecov-io commented Apr 7, 2020

Codecov Report

Merging #11011 into master will increase coverage by 0.52%.
The diff coverage is 84.12%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #11011      +/-   ##
==========================================
+ Coverage   60.76%   61.29%   +0.52%     
==========================================
  Files         585      591       +6     
  Lines       31947    32378     +431     
  Branches     4527     4586      +59     
==========================================
+ Hits        19414    19847     +433     
+ Misses      11546    11528      -18     
- Partials      987     1003      +16     
Impacted Files Coverage Δ
src/client/activation/serviceRegistry.ts 83.54% <ø> (-0.61%) ⬇️
src/client/common/application/types.ts 100.00% <ø> (ø)
...erpreter/configuration/pythonPathUpdaterService.ts 23.07% <0.00%> (ø)
src/client/interpreter/configuration/types.ts 100.00% <ø> (ø)
src/client/telemetry/index.ts 87.38% <ø> (ø)
src/client/common/application/workspace.ts 36.66% <40.00%> (+5.89%) ⬆️
src/client/common/utils/cacheUtils.ts 83.33% <53.33%> (-6.15%) ⬇️
src/client/interpreter/interpreterService.ts 65.94% <67.74%> (+4.67%) ⬆️
src/client/common/configSettings.ts 69.81% <69.81%> (+8.91%) ⬆️
src/client/interpreter/display/index.ts 88.23% <80.00%> (-2.88%) ⬇️
... and 45 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d2e0b7b...83d8e66. Read the comment docs.

@karrtikr karrtikr force-pushed the pythonPath branch 3 times, most recently from ee7c936 to 83d8e66 Compare April 8, 2020 17:31
package.nls.json Outdated Show resolved Hide resolved
Kartik Raj and others added 6 commits April 8, 2020 10:50
…nd (#10373)

* Modified `Select interpreter` command to support setting interpreter at workspace level

* Added a command to reset value of Python interpreter

* Code reviews

* Update src/client/interpreter/configuration/interpreterSelector.ts

Co-Authored-By: Karthik Nadig <kanadig@microsoft.com>

Co-authored-by: Karthik Nadig <kanadig@microsoft.com>
…10325)

* Added python.defaultInterpreterPath setting

* Created persistent storage infrastructure

* Added vscode.workspace.workspaceFile API

* Implemented inspect interpreter path

* Implement config service updater for pythonPath

* Correct build errors

* Change extension to use new infrastructure

* Store service container in a global variable

* Update settings in the new way src/test/common

* Correct cyclic dependency between IExperimentsManager and IConfigurationService

* Detect change in new interpreter storage and act accordingly

* Added reset python interpreters command

* Cache the auto selected interpreter

* Log python interpreter in the output channel

* Fix autoselection and speed up the interpreter change process

* Reset experiments

* Reset python interpreters for workspace

* Remove reset interpreter command from this PR

* Added news entry

* Update getWorkspaceFolderIdentifier

* Code reviews

* Dispose handler

* More code reviews

* Resolve merge conflicts

* Fix issues

* Some more fixes

* More fixes

* Do not assume ACTIVATED_SERVICE_CONTAINER is defined

* Fix running tests

* Fix cacheUtils.ts and test/common.ts

* Correct cacheResourceSpecificInterpreterData decorator

* Added tests for decorator-like implementation in enviroment provider

* Code reviews

* Fix interpreter service unit tests

* Fix interpreter display unit tests

* Fix mac interpreter unit tests

* Fix interpreter selector unit tests

* Fix configSettings pythonPath unit tests

* Fix configSettings unit tests

* Rebase with master

* Fix installer.test.ts tests

* Fix moduleInstaller.test.ts tests

* Fix pythonPathUpdater.test.ts tests

* Fix pythonProc.simple.multiroot.test.ts tests

* Fix data science functional tests

* Fix smoke tests

* Use user friendly path in the output channel

* Use symbols directly

* Fix bug with configSettings.ts

* Fix venv tests

* Run all tests with the new interpreter storage"

* Restore YAML pipeline

* Fix absolute path resolver

* Run all tests with old interpreter storage

* Reduce run time of venv tests

* Run only mac multiroot

* Remove experiment from experiments.json

* Added tests for Interpreter path service

* saa

* Ensure we use `CI_PYTHON_PATH` when running tests with the new interpreter storage

* Reset global interpreter path as well

* Added tests for mac interpreter diagnostic

* Added tests for src\client\interpreter\autoSelection\rules\settings.ts

* Added tests for src\client\interpreter\configuration\pythonPathUpdaterServiceFactory.ts

* Added tests for src\client\interpreter\autoSelection\rules\workspaceEnv.ts

* Added tests for src\client\interpreter\interpreterService.ts

* Added tests for src\client\common\configuration\service.ts

* Added tests for src\client\startupTelemetry.ts

* Correct tests

* Module installer tests

* Code reviews

* More code reviews

* Fix lint error
…ected (#10430)

* Implement prompt when an unsafe workspace python environment is to be autoselected

* Code reviews

* Refactor pythonPath stuff into a private method

* Use variable instead of hardcoding 'python' string

* Added tests for src\client\interpreter\autoSelection\interpreterSecurity\interpreterSecurityCommands.ts

* Don't register command in the constructor

* Refactored code

* Updated tests

* Code reviews

* Moved bannerLabelYes/No to Common() namespace

* Update content for news entry

* Corrected activation manager tests

* Fix single workspace CI tests

* Fix multiroot tests + Code reviews

* Rename Interpreter Security clear command

* Fix tests

* Added intellisense for pythonpath experiment

* Fix multiroot tests

* Rename `Reset interpreter` command to `Clear workspace interpreter setting

* Fix bug with interpreter security storage
@sonarcloud
Copy link

sonarcloud bot commented Apr 8, 2020

SonarCloud Quality Gate failed.

Bug C 1 Bug
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

return false;
}
}
'use strict';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
'use strict';
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.
'use strict';

Is this missing the copyright at the top?

Copy link
Member

Choose a reason for hiding this comment

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

That is OK if it is missing on a lot of files. we can just add it separately.

Copy link
Author

Choose a reason for hiding this comment

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

I think it's missing from the beginning. We probably shouldn't add it as Don authored the file when he was not affiliated with Microsoft.

@karrtikr karrtikr merged commit d95f454 into master Apr 8, 2020
@karrtikr karrtikr deleted the pythonPath branch April 8, 2020 18:29
@lock lock bot locked as resolved and limited conversation to collaborators Apr 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants