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

Add ph5ws.resif.fr endpoint #2938

Merged
merged 6 commits into from
Jan 11, 2022
Merged

Add ph5ws.resif.fr endpoint #2938

merged 6 commits into from
Jan 11, 2022

Conversation

jschaeff
Copy link
Contributor

@jschaeff jschaeff commented Jan 7, 2022

What does this PR do?

  • Update the RESIF service enpoints in the obspy.clients
  • Add new RESIFPH5 service endpoint

Why was it initiated? Any relevant Issues?

  • Résif-DC has now a second endpoint to serve Large-N data : RESIFPH5

I don't know if any testing is relevant for this PR.

It is a bit linked to issue #2914

PR Checklist

  • Correct base branch selected? master for new features, maintenance_... for bug fixes
  • This PR is not directly related to an existing issue (which has no PR yet).
  • If the PR is making changes to documentation, docs pages can be built automatically.
    Just remove the space in the following string after the + sign: "+ DOCS"
  • If any network modules should be tested for the PR, add them as a comma separated list
    (e.g. clients.fdsn,clients.arclink) after the colon in the following magic string: "+TESTS:"
    (you can also add "ALL" to just simply run all tests across all modules)
  • All tests still pass.
  • Any new features or fixed regressions are be covered via new tests.
  • Any new or changed features have are fully documented.
  • Significant changes have been added to CHANGELOG.txt .
  • First time contributors have added your name to CONTRIBUTORS.txt .

Copy link
Member

@claudiodsf claudiodsf left a comment

Choose a reason for hiding this comment

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

I don't think testing is important for this one. Let's just check that CI succeeds (no reason to fail), then I'll merge.

Copy link
Member

@claudiodsf claudiodsf left a comment

Choose a reason for hiding this comment

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

If you want you can add a Changelog line, like this one

@jschaeff
Copy link
Contributor Author

jschaeff commented Jan 7, 2022

damn, conflict in the changelog :(

@claudiodsf
Copy link
Member

damn, conflict in the changelog :(

You need to rebase and force-push :-)

@claudiodsf
Copy link
Member

And don't forget to add yourself to CONTRIBUTORS.txt !

@jschaeff
Copy link
Contributor Author

jschaeff commented Jan 7, 2022

I made a mess but I guess it's OK now.

@claudiodsf
Copy link
Member

Don't worry. Let's see tests, than I'll squash-and-merge™

@d-chambers
Copy link
Member

Good to merge?

@claudiodsf
Copy link
Member

Good to merge?

Nope, still one pending question.

@jschaeff
Copy link
Contributor Author

I missed the still pending question, sorry. What was it ?

@ThomasLecocq
Copy link
Contributor

@claudiodsf ?

@claudiodsf
Copy link
Member

I missed the still pending question, sorry. What was it ?

pending_question

@ThomasLecocq
Copy link
Contributor

fun, I didn't see this one!

@claudiodsf
Copy link
Member

fun, I didn't see this one!

Do you see it now?

@ThomasLecocq
Copy link
Contributor

negative, when I go to the files it's not there - did you "submit your review" ?

CHANGELOG.txt Outdated
@@ -41,7 +41,7 @@ Changes:
FDSN server URLs (#2878)
* add URL mapping for IRISPH5, IESDMC, GEOFON (alternative to GFZ)
(see #2739, #2932)
* update RESIF URL mapping to use https
* update RESIF URL mapping to use https and add RESIFPH5 (see #2938)
Copy link
Member

Choose a reason for hiding this comment

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

Actually it seems to be the other way round: it used to be "https", now it is "http".
Could you check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I corrected the changelog, sorry about this :)

@claudiodsf
Copy link
Member

negative, when I go to the files it's not there - did you "submit your review" ?

I guess that was the problem!

@QuLogic
Copy link
Member

QuLogic commented Jan 10, 2022

Pending means you haven't posted it, not that it hasn't been answered.

@claudiodsf claudiodsf merged commit bdb935c into obspy:master Jan 11, 2022
@claudiodsf
Copy link
Member

Merged! Thanks @jschaeff!

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.

None yet

5 participants