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

Support for https endpoints #103

Closed
wants to merge 3 commits into from
Closed

Support for https endpoints #103

wants to merge 3 commits into from

Conversation

rodolfomiranda
Copy link
Collaborator

This PR replace PR #91 due branch inconsistencies after moving from main to development.

@codecov
Copy link

codecov bot commented Sep 13, 2023

Codecov Report

Merging #103 (68f5a99) into development (05c7eab) will increase coverage by 1.50%.
Report is 6 commits behind head on development.
The diff coverage is 84.11%.

@@               Coverage Diff               @@
##           development     #103      +/-   ##
===============================================
+ Coverage        88.20%   89.70%   +1.50%     
===============================================
  Files               30       34       +4     
  Lines             4789     5257     +468     
===============================================
+ Hits              4224     4716     +492     
+ Misses             565      541      -24     
Files Changed Coverage Δ
tests/app/test_specing.py 100.00% <ø> (ø)
src/keria/app/grouping.py 22.36% <22.36%> (ø)
src/keria/peer/exchanging.py 30.55% <30.55%> (ø)
src/keria/end/ending.py 85.71% <50.00%> (+54.34%) ⬆️
src/keria/app/aiding.py 85.03% <83.87%> (+7.29%) ⬆️
src/keria/app/agenting.py 84.10% <100.00%> (+2.78%) ⬆️
src/keria/app/credentialing.py 79.00% <100.00%> (+0.73%) ⬆️
src/keria/app/notifying.py 100.00% <100.00%> (ø)
src/keria/app/presenting.py 96.61% <100.00%> (+1.69%) ⬆️
src/keria/testing/testing_helper.py 99.29% <100.00%> (ø)
... and 8 more

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@m00sey m00sey left a comment

Choose a reason for hiding this comment

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

tests

@@ -889,31 +888,34 @@ def on_get(req, rep, alias):
if role in (kering.Roles.witness,): # Fetch URL OOBIs for all witnesses
oobis = []
for wit in hab.kever.wits:
urls = hab.fetchUrls(eid=wit, scheme=kering.Schemes.http)
urls = hab.fetchUrls(eid=wit, scheme=kering.Schemes.http) or hab.fetchUrls(eid=wit, scheme=kering.Schemes.https)
Copy link
Member

Choose a reason for hiding this comment

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

should add a test to cover this branch in logic

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. We don't have coverage of that branch. In order to fully test that branch it seems we need to bring in locally self signed certificates for proper TLS. I could bring that in from HIO like I mentioned. Would you still prefer mocking in this case? We'd just need a hab with TLS urls. I'm not familiar with the test fixtures though I suppose there is a Hab test fixture pretty close to what we need.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, looks like we'll need a new helpers.openKeria() test fixture that returns an agency started up with TLS config. Want me to make that for this PR?

@m00sey m00sey assigned m00sey and rodolfomiranda and unassigned m00sey Sep 14, 2023
@kentbull
Copy link
Contributor

The main difference between these two PRs was the usage of urljoin to combine the parsed URL up with the rest of the OOBI URL string. I added that to #103 and we can merge that.

@rodolfomiranda
Copy link
Collaborator Author

I'm closing this PR since all the changes were included in PR #102

@rodolfomiranda
Copy link
Collaborator Author

I'm closing this PR since all the changes were included in PR #102

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.

3 participants