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

Parse envFile and pass to debugger #1951

Closed
wants to merge 3 commits into from

Conversation

SebastianPfliegel
Copy link
Contributor

@SebastianPfliegel SebastianPfliegel commented Jan 4, 2018

Closes #1944. Adds a new option for launch.json - envFile: it will read environment variables from the given UTF8 file. This way environment variables can be passed to the debugger and launch.json stays clean of sensitive data (like passwords).

WIP

  • Remove UTF8-BOM if available
  • Ignore comments (using #)
  • Add similar logic for args

@dnfclas
Copy link

dnfclas commented Jan 4, 2018

CLA assistant check
All CLA requirements met.

@gregg-miskelly
Copy link
Contributor

Looks like a good start to me

@gregg-miskelly
Copy link
Contributor

@SebastianPfliegel is this ready for review or are you planning to add support for more things first?

@SebastianPfliegel
Copy link
Contributor Author

It's still WIP. I'll try to finish it today or tomorrow.

@gregg-miskelly
Copy link
Contributor

@SebastianPfliegel No rush on this, but just to let you know -- we are just about to start locking down for our next release (1.14). So if you get this done by early next week we can probably include it. Else, no worries, it can ship in a later release.

@giard-alexandre
Copy link

giard-alexandre commented Apr 16, 2018

Any updates on this PR? Since we're using Docker-compose and a .env file at work, this would be a GREAT way for us to consolidate both configs as much as possible. Thanks! 😄

@SebastianPfliegel
Copy link
Contributor Author

@heuristicAL, great to hear my PR is of use. I wasn't able to finish it as dotNet core was broken for my Linux distro, but it has been fixed last week, so I'll give it a try this week to finish it :)

@giard-alexandre
Copy link

@SebastianPfliegel Great news! If you need any help let me know, I'm willing to give this a try, although my experience developing extensions for VS Code amounts to about.... 0. 👍

@SebastianPfliegel
Copy link
Contributor Author

Haha, was the same for me. That's my first contribution regarding JavaScript/TypeScript ;)

@irq
Copy link

irq commented May 9, 2018

@SebastianPfliegel Eagerly awaiting this, any updates?

@giard-alexandre
Copy link

Any updates on this? Would I be wasting my time if I were to fork out and update the PR?

@codecov
Copy link

codecov bot commented Jul 17, 2018

Codecov Report

Merging #1951 into master will decrease coverage by 0.3%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1951      +/-   ##
==========================================
- Coverage   63.63%   63.33%   -0.31%     
==========================================
  Files          88       88              
  Lines        4007     4020      +13     
  Branches      567      570       +3     
==========================================
- Hits         2550     2546       -4     
- Misses       1294     1311      +17     
  Partials      163      163
Flag Coverage Δ
#integration 52.65% <0%> (-0.03%) ⬇️
#unit 84.66% <ø> (-0.06%) ⬇️
Impacted Files Coverage Δ
src/configurationProvider.ts 17.24% <0%> (-10.54%) ⬇️
src/observers/OmnisharpLoggerObserver.ts 100% <0%> (ø) ⬆️
src/omnisharp/options.ts 100% <0%> (ø) ⬆️
src/omnisharp/launcher.ts 54.16% <0%> (+0.94%) ⬆️
src/observers/CsharpLoggerObserver.ts 100% <0%> (+3.63%) ⬆️

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 82be5e1...aa3214f. Read the comment docs.

@SebastianPfliegel
Copy link
Contributor Author

@heuristicAL & @irq: sorry for letting you wait. I squashed the commits and tested the code thoroughly. It was already skipping remarks (beginning with #).

Here's some examples how the .env file gets parsed:

TEST1=singlevalue
TEST2=two words
TEST3="hey"
TEST4='another test'
TEST5 = space
TEST6      =      lots of space
   TEST7 = space at beginning

#TEST9=just a remark
/TEST10=doesntworkeither

And this is the result:

TEST1: singlevalue
TEST2: two words
TEST3: hey
TEST4: another test
TEST5: space
TEST6: lots of space
TEST7: space at beginning
empty line gets skipped
TEST9 won't parse
TEST10 won't parse

@gregg-miskelly: care to review? :)

Copy link
Contributor

@gregg-miskelly gregg-miskelly left a comment

Choose a reason for hiding this comment

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

Aside from those two small changes, looks good to me. Thanks for taking the time to finish this up.

content.split("\n").forEach(line => {
const r: RegExpMatchArray = line.match(/^\s*([\w\.\-]+)\s*=\s*(.*)?\s*$/);

if (r !== null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we provide some sort of error message if this is null?

Copy link
Contributor

Choose a reason for hiding this comment

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

(Excluding lines that start with '#')


In reply to: 203184365 [](ancestors = 203184365)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also done!

@@ -6021,5130 +6021,6 @@
"once": "1.4.0"
}
},
"npm": {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you want to backout your changes to this file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@SebastianPfliegel
Copy link
Contributor Author

Committed your requested changes. Should I squash them?

@gregg-miskelly
Copy link
Contributor

Yes please.

@SebastianPfliegel
Copy link
Contributor Author

Consider it done ;)

@SebastianPfliegel
Copy link
Contributor Author

@gregg-miskelly: were you able to test it?

@gregg-miskelly
Copy link
Contributor

@SebastianPfliegel Sorry for the delay. We had CI issues last week and I didn't have time to investigate. I will see if I can get this merged now...

@gregg-miskelly
Copy link
Contributor

@akshita31: Would you mind merging this for me once the CI issues are fixed? I am going on vacation. If you need help from the debugger team chuckries is going to try and monitor things.

@akshita31
Copy link
Contributor

@gregg-miskelly Sure, we are reverting to an older omnisharp version instead of the "latest" one for the integration tests. Will merge this, once we get the CI working.

@akshita31
Copy link
Contributor

@SebastianPfliegel The CI issue has been fixed, but the build still fails. I pulled your branch and executed an npm run compile, and found that there is a compile error that is causing the build to fail.

@gregg-miskelly
Copy link
Contributor

@SebastianPfliegel thanks so much for doing this! I made more changes to get this to work and created a new PR with both our work -- #2462

I will close this one.

If you have the time, feel free to review my PR.

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.

6 participants