-
Notifications
You must be signed in to change notification settings - Fork 7
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
Adding service information for the STAC catalog #373
Conversation
Can one of the admins verify this patch? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with other comments from @mishaschwartz
I think all suggestions and requests are addressed now. |
{ | ||
"rel": "service-desc", | ||
"type": "application/yaml", | ||
"href": "https://raw.githubusercontent.com/radiantearth/stac-api-spec/main/core/openapi.yaml" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better actually to refer to a pinned version (release/tag).
https://github.com/radiantearth/stac-api-spec/tree/v1.0.0 should be good.
We should probably make this a configurable env-var with the corresponding app tag for the docker image.
Right now, crim-ca/stac-app:main
is used as well:
https://github.com/bird-house/birdhouse-deploy/blob/master/birdhouse/components/stac/docker-compose-extra.yml#L13
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree about the pinned version. I didn't quite understand your second comment..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fmigneault can this be addressed in a different PR? It's a good point but probably should be addressed later when we actually start using a version other than crim-ca/stac-app:main
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I agree to move it to another PR while addressing the reference image.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs update to .gitignore and info in stac-browser/service-config.json
doesn't actually get added to the json string.
(I can work on this)
EDIT: fixed
@fmigneault @tlvu @dchandan I had to relax the tests to permit a service config file to allow the name of any service in the file to be the same as the component folder since we have to now support multiple services per component (the The other option is to treat both stac and stac-browser as the same service and include the url for one of them in the
I chose What is your preferred strategy for this? I'm leaning towards allowing multiple services per component because it allows us more flexibility and I'm having a hard time seeing a downside to this. |
@mishaschwartz |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Waiting on follow-up on #373 (comment) and #373 (comment).
Just to clarify:
Do you recommend two services (stac and stac-browser) with links referring to each other, or do you recommend one service (stac-browser) with a link to the stac endpoint? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Two services with links referring to each other since they are related, but still two distinct entities. Each service in this case has different references: For the test validation portion, matching at least 1 service by name seems reasonable. Cross-links could also be validated. |
Overview
STAC is a new service to the birdhouse-stack and it should be made visible at the
/services
endpoint. Currently this is missing.Changes
Non-breaking changes
service-config.json.template
file tobirdhouse/components/stac
.Breaking changes
N/A