Skip to content

feat(cli): add DENO_CERT environment variable#6370

Merged
ry merged 4 commits intodenoland:masterfrom
linde12:master
Jul 12, 2020
Merged

feat(cli): add DENO_CERT environment variable#6370
ry merged 4 commits intodenoland:masterfrom
linde12:master

Conversation

@linde12
Copy link
Copy Markdown
Contributor

@linde12 linde12 commented Jun 18, 2020

Add the ability to specify CA path in environment variable DENO_CERT. Running deno with the --cert arg will take precedence over DENO_CERT

See #6355

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Jun 18, 2020

CLA assistant check
All committers have signed the CLA.

@ry
Copy link
Copy Markdown
Member

ry commented Jun 19, 2020

Thanks for the patch @linde12 - we need a test of this... You should be able to copy and modify "cafile_fetch" in cli/tests/integration_tests.rs for this env var case.

@ry ry added this to the 1.2.0 milestone Jun 19, 2020
@linde12
Copy link
Copy Markdown
Contributor Author

linde12 commented Jun 19, 2020

Thanks for the tip @ry, i was wondering how i would test this. I will get back to this and add a test in a day or two if that is ok, im currently celebrating a holiday! :-)

@ry
Copy link
Copy Markdown
Member

ry commented Jun 19, 2020

Thanks - have a happy holiday!

BTW we won't be able to release this feature until 1.2.0, which is scheduled for July 13 - so no rush.

@linde12
Copy link
Copy Markdown
Contributor Author

linde12 commented Jun 19, 2020

Thanks! Plenty of time then hehe, but hopefully i'll have time this weekend. Thanks for letting me know👍

@linde12
Copy link
Copy Markdown
Contributor Author

linde12 commented Jun 21, 2020

Not sure if we want to test the precedence also, but i added a test based upon cafile_fetch

@linde12
Copy link
Copy Markdown
Contributor Author

linde12 commented Jun 24, 2020

CC @ry what do you think? 😃

@bartlomieju bartlomieju added cli related to cli/ dir feat new feature (which has been agreed to/accepted) labels Jun 24, 2020
let deno_dir = TempDir::new().expect("tempdir fail");
let module_url =
Url::parse("http://localhost:4545/cli/tests/cafile_url_imports.ts")
.unwrap();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this URL should be https://localhost:5545/cli/tests/cafile_info.ts

I see that cafile_fetch below is also using a http URL rather than https. I think that is an error.

What do you think?

Copy link
Copy Markdown
Contributor Author

@linde12 linde12 Jun 24, 2020

Choose a reason for hiding this comment

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

@ry Nice catch! The weird thing is that test is actually failing if i remove the DENO_CERT env (i tested with and without it before submitting the PR) so something weird is happening? Could it be that the connection is being upgraded to HTTPS? 🤷

I changed to https://localhost:5545/cli/tests/cafile_url_imports.ts and it works, or did you specifically want cafile_info.ts? Also, want me to update the cafile_fetch test in the same PR?

- make cafile_env_fetch target https url
- don't assert on stdout, just assert status
@linde12
Copy link
Copy Markdown
Contributor Author

linde12 commented Jun 24, 2020

Tests were killed with SIGABRT and AFAIK there is no way to manually retrigger?

@linde12 linde12 requested a review from ry June 30, 2020 13:10
@bartlomieju
Copy link
Copy Markdown
Member

Tests were killed with SIGABRT and AFAIK there is no way to manually retrigger?

@linde12 that might have been an intermittent failure. I've merged latest code to your branch let's see if it happens again.

@linde12
Copy link
Copy Markdown
Contributor Author

linde12 commented Jul 12, 2020

@bartlomieju Great, seems to be passing now👍 Can we merge or is there anything you want me to change before?

Copy link
Copy Markdown
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

LGTM - thank you @linde12

@ry ry merged commit 3be2064 into denoland:master Jul 12, 2020
@tdillon
Copy link
Copy Markdown

tdillon commented Jul 17, 2020

Is the DENO_CERT environment variable only being applied to the deno run command? Shouldn't it be used for all commands (e.g., deno upgrade)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cli related to cli/ dir feat new feature (which has been agreed to/accepted)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants