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

Towards VOResource 1.1 #269

Merged
merged 6 commits into from
Oct 29, 2021
Merged

Towards VOResource 1.1 #269

merged 6 commits into from
Oct 29, 2021

Conversation

msdemlei
Copy link
Contributor

This PR adds a few items to the VOResource Interface parser in pyvo.io.vosi. It would fix bug #268.

@msdemlei msdemlei changed the title To VORResource 1 1 To VOResource 1 1 Aug 11, 2021
@msdemlei msdemlei changed the title To VOResource 1 1 Towards VOResource 1.1 Aug 11, 2021
@codecov
Copy link

codecov bot commented Aug 11, 2021

Codecov Report

Merging #269 (01ace56) into master (fb16970) will increase coverage by 0.08%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #269      +/-   ##
==========================================
+ Coverage   74.96%   75.05%   +0.08%     
==========================================
  Files          44       44              
  Lines        5065     5083      +18     
==========================================
+ Hits         3797     3815      +18     
  Misses       1268     1268              
Impacted Files Coverage Δ
pyvo/io/vosi/voresource.py 82.65% <100.00%> (+2.01%) ⬆️

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 fb16970...01ace56. Read the comment docs.

@andamian
Copy link
Contributor

@msdemlei - is this a draft or it's ready for review?

@msdemlei
Copy link
Contributor Author

msdemlei commented Aug 20, 2021 via email

@andamian andamian self-requested a review October 28, 2021 21:28
@andamian
Copy link
Contributor

andamian commented Oct 28, 2021

@msdemlei - finally finished this. It looks good to me. Had to change the PR number in the change log (it needs the PR# and not the issue#) and pull master that has a fix to documentation but it's all green now. Can I merge it?

I assume there's follow up work on this to add support for mirror URLs and example query string:

  1. Should we add examples properties to all dal services?
  2. How are clients supposed to use the mirror URLs? I assume they can do an availability check when the class (SIAService, TAPService, etc.) is first instantiated and if it doesn't work try mirrors. Problem with this is the overhead of the call and the fact that mirror URLs are not linked between capabilities (how can the client tell that a mirror URL of the availability capability corresponds to a mirror URL of a different capability. Are they supposed to have the same title?). A second approach would be to try mirror URLs only on some HTTP Errors. The trick here is to identify which errors (there's no point in trying a malformed query on different mirror URLs - the result will be the same).

Anyways, we could close this and track follow-ups with new issues.

@msdemlei
Copy link
Contributor Author

msdemlei commented Oct 29, 2021 via email

@andamian andamian merged commit a612045 into master Oct 29, 2021
@andamian andamian deleted the to-voresource-1_1 branch October 29, 2021 15:43
@andamian
Copy link
Contributor

Merged. Thank you!

mirrorURL: I've made a comment to the GMS RFC that the GMS is probably a prime candidate of this feature. GMS plays a critical role in the authentication and its high availability is critical. Service providers should probably try to deploy alternative mirrors to ensure the service is always reachable on at least one of the sites.

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

Successfully merging this pull request may close these issues.

2 participants