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

fix(cli): ca and certs are not included in loading db from CLI #8111

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

fix(cli): ca and certs are not included in loading db from CLI #8111

wants to merge 5 commits into from

Conversation

ihexxa
Copy link
Contributor

@ihexxa ihexxa commented Oct 22, 2024 •
edited
Loading

Changes

  • fix: ca and certs are not included in loading db
  • Added a test for cert db loading
  • Added a test for settings db loading

Ref: INS-4598

@ihexxa ihexxa self-assigned this Oct 22, 2024
@ihexxa ihexxa changed the title fix: ca and certs are not included in loading db fix(CLI): ca and certs are not included in loading db from CLI Oct 22, 2024
@ihexxa ihexxa changed the title fix(CLI): ca and certs are not included in loading db from CLI fix(cli): ca and certs are not included in loading db from CLI Oct 22, 2024
filfreire
filfreire previously approved these changes Oct 22, 2024
View reviewed changes
@filfreire filfreire requested review from gatzjames, CurryYangxx and a team October 22, 2024 10:48
@ihexxa ihexxa enabled auto-merge (squash) October 22, 2024 11:14
ryan-willis
ryan-willis previously approved these changes Oct 22, 2024
View reviewed changes
jackkav
jackkav reviewed Oct 23, 2024
View reviewed changes
Copy link
Contributor

@jackkav jackkav left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Is this just a plumbing PR or is there a case that works now that didn't before? The word fix implies something wasn't working before, if that is the case there probably could be a test case to show what it was.

@ihexxa
Copy link
Contributor Author

ihexxa commented Oct 23, 2024 •
edited
Loading

Let me take a look and cover it, originally it was a quick fix but as more changes will be introduced, setting it to draft.

@ihexxa ihexxa marked this pull request as draft October 23, 2024 07:45
auto-merge was automatically disabled October 23, 2024 07:45

Pull request was converted to draft

@ihexxa ihexxa dismissed stale reviews from ryan-willis and filfreire via 82b4275 October 24, 2024 08:52
@ihexxa
Copy link
Contributor Author

ihexxa commented Oct 25, 2024

Added tests with fake db, certs. The cert test will load the fake cert although it will finally fail, which should be ok.
And the settings test will load settings db and doesn't follow the 302 redirection as allowRedirect is disabled in settings. I choose testing this option because almost any setting potentially could affect existing tests' behavior.

@ihexxa ihexxa marked this pull request as ready for review October 25, 2024 07:16
filfreire
filfreire reviewed Oct 25, 2024
View reviewed changes
packages/insomnia-inso/src/db/fixtures/certs/fake_key.pem
@@ -0,0 +1 @@
fake key content
Copy link
Member

Choose a reason for hiding this comment

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

for a future PR pass I recommend we also try with a working cert

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1 originally I added one but the key was blamed by the github bot above, probably we could try with cert. 🥹

filfreire
filfreire reviewed Oct 25, 2024
View reviewed changes
packages/insomnia-inso/src/cli.test.ts
console.log(result);
}
expect(result.stdout).toContain('Adding SSL PEM certificate');
expect(result.stdout).toContain('Adding SSL KEY certificate');
Copy link
Member

Choose a reason for hiding this comment

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

👍

@ihexxa ihexxa enabled auto-merge (squash) October 25, 2024 09:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jackkav jackkav jackkav left review comments

@filfreire filfreire filfreire approved these changes

@ryan-willis ryan-willis ryan-willis left review comments

@gatzjames gatzjames Awaiting requested review from gatzjames

@CurryYangxx CurryYangxx Awaiting requested review from CurryYangxx

Assignees

@ihexxa ihexxa

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ihexxa @jackkav @filfreire @ryan-willis