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.

Sign up for GitHub

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

Jump to bottom

ProcessService needs to be modified to use resolved environment variables #1339

Closed
2 tasks
DonJayamanne opened this issue Apr 7, 2018 · 1 comment · Fixed by #1342
Closed
2 tasks

ProcessService needs to be modified to use resolved environment variables #1339

DonJayamanne opened this issue Apr 7, 2018 · 1 comment · Fixed by #1342
Assignees
@DonJayamanne
Labels
debt Covers everything internal: CI, testing, refactoring of the codebase, etc.
Milestone
May 2018

Comments

@DonJayamanne
Copy link

DonJayamanne commented Apr 7, 2018 •
edited
Loading

When env variables are not passed into methods of IProcessService, we use the current process env variables.
However, these variables do not contain variables defined in the environment files.

E.g. issues such as #1338 cannot be resolved even if .env file has been updated.

  • Create tests
  • Create tests to ensure existing code will also utilize these new variables (defined in .env files)
@DonJayamanne DonJayamanne self-assigned this Apr 7, 2018
@DonJayamanne DonJayamanne added bug Issue identified by VS Code Team member as probable bug needs PR debt Covers everything internal: CI, testing, refactoring of the codebase, etc. and removed bug Issue identified by VS Code Team member as probable bug labels Apr 7, 2018
@DonJayamanne DonJayamanne added this to the April 2018 milestone Apr 7, 2018
@DonJayamanne
Copy link
Author

DonJayamanne commented Apr 8, 2018 •
edited
Loading

Seems like a very large PR

Option 1

  • Creating separate PRs
  • With additional tests
    Lets discuss the need for this later during the week.

Option 2

  • Always pass environment variables to the exec and execObservable methods of IProcessService
  • Ensure to check this in PRs.

@DonJayamanne DonJayamanne mentioned this issue Apr 8, 2018
Merged
5 tasks
@DonJayamanne DonJayamanne modified the milestones: April 2018, May 2018 Apr 18, 2018
DonJayamanne added a commit that referenced this issue May 8, 2018
@DonJayamanne
Add factory class to create the process service (#1342)
128d231 
* Fixes #1339
* factory to create the process service
* 🔨 pass resource
* 📝 news entry
* remove process service
* 🔨 use factory to create the service
* Remove dependency on IProcessService
* Remove use of env provider service
* Revert changes
* Fix merge issue
@lock lock bot locked as resolved and limited conversation to collaborators Jul 11, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@DonJayamanne DonJayamanne

Labels
debt Covers everything internal: CI, testing, refactoring of the codebase, etc.
Projects
None yet
Milestone
May 2018
Development

Successfully merging a pull request may close this issue.

Add factory class to create the process service DonJayamanne/pythonVSCode
1 participant
@DonJayamanne