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

terminal: normalize envs before merging #9631

Merged
merged 1 commit into from
Jun 29, 2021
Merged

terminal: normalize envs before merging #9631

merged 1 commit into from
Jun 29, 2021

Conversation

paul-marechal
Copy link
Member

@paul-marechal paul-marechal commented Jun 22, 2021 •
edited
Loading

Environment variables are case insensitive on Windows so we'll align
everything to use uppercase when on Windows.

How to test

Not really sure. My PATH environment variable on Windows is named Path in process.env for some reason. This caused issues when running the @theia/terminal test suite as at some point we expect to fetch PATH from an object that should look like process.env but failed as the name was Path instead of PATH in that new js object.

Review checklist

Reminder for reviewers

@paul-marechal paul-marechal added bug bugs found in the application terminal issues related to the terminal process issues related to the process extension labels Jun 22, 2021
@paul-marechal
Copy link
Member Author

cc @alvsan09

@alvsan09
Copy link
Contributor

Case should not be important, so we'll align everything to
use uppercase.

My understanding is that the applications that use the variable may rely on the original casing, it's out of our control.
if that's the case, we should preserve the casing. Would you agree?

@paul-marechal
Copy link
Member Author

The general wisdom is that environment variables should be uppercased. When it comes to process.env, it is a special object which will ignore the case when accessing members, e.g. process.env.PATH is equivalent to process.env.Path or process.env.path. Since we create new objects which are not special like process.env is, we have to pick a case. In this case, it's uppercase.

@paul-marechal paul-marechal force-pushed the mp/merge-env-var-name branch from 8d75595 to b0b8c54 Compare June 23, 2021 15:01
vince-fugnitto
vince-fugnitto reviewed Jun 23, 2021
View reviewed changes
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

@paul-marechal is there a need to add tests to shell-process.spec.ts to ensure the method works successfully under different conditions, and for different operating systems?

alvsan09
alvsan09 reviewed Jun 23, 2021
View reviewed changes
Copy link
Contributor

@alvsan09 alvsan09 left a comment

Choose a reason for hiding this comment

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

just a couple of suggestions.

packages/terminal/src/node/shell-process.ts Outdated Show resolved Hide resolved
packages/terminal/src/node/shell-process.ts Outdated Show resolved Hide resolved
@paul-marechal paul-marechal force-pushed the mp/merge-env-var-name branch 3 times, most recently from 9a1d1ab to 598c094 Compare June 23, 2021 16:24
@paul-marechal
Copy link
Member Author

I renamed uppercaseEnv to normalizeEnv and added tests for it.

@paul-marechal paul-marechal force-pushed the mp/merge-env-var-name branch 2 times, most recently from 0d87984 to c046bd2 Compare June 23, 2021 16:46
@paul-marechal paul-marechal force-pushed the mp/merge-env-var-name branch 2 times, most recently from b3a69dc to 0e14c97 Compare June 23, 2021 18:37
alvsan09
alvsan09 reviewed Jun 23, 2021
View reviewed changes
Copy link
Contributor

@alvsan09 alvsan09 left a comment

Choose a reason for hiding this comment

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

It looks good to me,
Just a typo, see inline comment!

packages/core/src/node/environment-utils.ts Outdated Show resolved Hide resolved
@paul-marechal paul-marechal force-pushed the mp/merge-env-var-name branch from 0e14c97 to 69b27d7 Compare June 23, 2021 19:28
alvsan09
alvsan09 approved these changes Jun 23, 2021
View reviewed changes
Copy link
Contributor

@alvsan09 alvsan09 left a comment

Choose a reason for hiding this comment

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

Looks go to me !! 👍

@paul-marechal paul-marechal force-pushed the mp/merge-env-var-name branch from 69b27d7 to 0b648f5 Compare June 23, 2021 22:16
colin-grant-work
colin-grant-work approved these changes Jun 23, 2021
View reviewed changes
Copy link
Contributor

@colin-grant-work colin-grant-work left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me. One minor comment and a question, but nothing to hold this up.

packages/core/src/node/environment-utils.ts Outdated Show resolved Hide resolved
packages/core/src/node/environment-utils.ts Show resolved Hide resolved
@paul-marechal paul-marechal force-pushed the mp/merge-env-var-name branch 2 times, most recently from 704a345 to d3bf0e0 Compare June 23, 2021 22:47
@paul-marechal
Copy link
Member Author

CI fails with HTTP error 503 service unavailable when downloading plugins...

@paul-marechal paul-marechal force-pushed the mp/merge-env-var-name branch from d3bf0e0 to 0b2f445 Compare June 28, 2021 20:08
@paul-marechal
terminal: normalize envs before merging
93caeb3 
Environment variables are case insensitive on Windows so we'll align
everything to use uppercase when on Windows.

Refactor `mergeProcessEnv` function into its own injectable
`EnvironmentUtils` class. This fixes the usage of raw exported functions
which might be hard to override for clients.
@paul-marechal paul-marechal force-pushed the mp/merge-env-var-name branch from 0b2f445 to 93caeb3 Compare June 28, 2021 23:41
@paul-marechal paul-marechal merged commit 075dfe2 into master Jun 29, 2021
@paul-marechal paul-marechal deleted the mp/merge-env-var-name branch June 29, 2021 05:10
@github-actions github-actions bot added this to the 1.15.0 milestone Jun 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vince-fugnitto vince-fugnitto vince-fugnitto left review comments

@colin-grant-work colin-grant-work colin-grant-work approved these changes

@alvsan09 alvsan09 alvsan09 approved these changes

Assignees
No one assigned
Labels
bug bugs found in the application process issues related to the process extension terminal issues related to the terminal
Projects
None yet
Milestone
1.15.0
Development

Successfully merging this pull request may close these issues.
4 participants
@paul-marechal @alvsan09 @vince-fugnitto @colin-grant-work