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

Adding service information for the STAC catalog #373

Merged
merged 14 commits into from
Oct 2, 2023
Merged

Conversation

dchandan
Copy link
Collaborator

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

  • Adding a service-config.json.template file to birdhouse/components/stac.

Breaking changes
N/A

@crim-jenkins-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@github-actions github-actions bot added documentation Improvements or additions to documentation feature/node-registry Related to https://github.com/DACCS-Climate/DACCS-node-registry labels Aug 18, 2023
Copy link
Collaborator

@fmigneault fmigneault left a 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

CHANGES.md Show resolved Hide resolved
@dchandan
Copy link
Collaborator Author

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"
Copy link
Collaborator

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

Copy link
Collaborator Author

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..

Copy link
Collaborator

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

Copy link
Collaborator

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.

Copy link
Collaborator

@tlvu tlvu left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@mishaschwartz mishaschwartz left a 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

@github-actions github-actions bot added the ci/operations Continuous Integration components label Sep 27, 2023
@github-actions github-actions bot added the ci/tests Issues or changes related to tests scripts label Sep 27, 2023
@mishaschwartz
Copy link
Collaborator

mishaschwartz commented Sep 28, 2023

@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 stac component offers two services: stac and stac-browser).

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 links key for the other. For example

{
  "name": "stac-browser",
  ...
  "links": [
      {"rel": "via",
       "type": "application/json",
       "href": "https://${PAVICS_FQDN_PUBLIC}/stac/"},
       ...
   ]
}

I chose via as the rel value since it "Identifies a resource that is the source of the information in the link's context." (see here) but I'm open to other suggestions.

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.

@fmigneault
Copy link
Collaborator

@mishaschwartz
I like the idea of linking the services between each other. The rel alternate could also be considered.

Copy link
Collaborator

@fmigneault fmigneault left a 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).

@mishaschwartz
Copy link
Collaborator

@fmigneault

Just to clarify:

I like the idea of linking the services between each other. The rel alternate could also be considered.

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?

Copy link
Collaborator

@tlvu tlvu left a comment

Choose a reason for hiding this comment

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

LGTM

@fmigneault
Copy link
Collaborator

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?

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: /stac/ pointing at STAC-API spec and crimca/stac-app implementation; while STAC-browser using the stac-utils/pgstac implementation.

For the test validation portion, matching at least 1 service by name seems reasonable. Cross-links could also be validated.
Service configs unmatched by name must still pass schema validation though.

@dchandan dchandan merged commit 33042e6 into master Oct 2, 2023
4 checks passed
@dchandan dchandan deleted the add-stac-service branch October 2, 2023 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci/operations Continuous Integration components ci/tests Issues or changes related to tests scripts documentation Improvements or additions to documentation feature/node-registry Related to https://github.com/DACCS-Climate/DACCS-node-registry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants