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

Nest raster, vector and stac services into apiServices. #161

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

pantierra
Copy link
Contributor

@pantierra pantierra commented Oct 29, 2024

This PR proposes encapsulating the external services—stac, raster, and vector—within an externalServices structure. This enhancement enables subcharts to extend these services, facilitating the injection of additional external services, such as an application frontend.

Involved in this PR @ciaransweet and @emmanuelmathot during our pod week.

@pantierra pantierra requested a review from ividito October 29, 2024 07:27
@pantierra pantierra marked this pull request as draft October 29, 2024 07:34
@pantierra
Copy link
Contributor Author

I will have to look into the failing tests, first.

@pantierra pantierra force-pushed the feature/extend-public-services branch from 4dfd7d2 to 7d9029c Compare October 29, 2024 07:48
@pantierra pantierra force-pushed the feature/extend-public-services branch from 2815058 to e1f756e Compare February 6, 2025 15:00
@pantierra pantierra force-pushed the feature/extend-public-services branch from 80a5351 to 91c83da Compare February 6, 2025 15:51
@pantierra pantierra marked this pull request as ready for review February 6, 2025 15:51
@pantierra pantierra requested a review from ciaransweet February 6, 2025 15:51
@pantierra
Copy link
Contributor Author

Just came back to this, as I wanted to add another service. I fixed the tests. It is ready for review. Thanks for looking into it.

@ividito
Copy link
Contributor

ividito commented Feb 6, 2025

Nit: externalServices -> eoAPIServices

Is there any way to use externalServices as a list of services to iterate over, but still keep the per-service values at the top level? It would be great to maintain some backwards compatibility, but I'm not sure how much control helm's templating gives us over that.

@pantierra
Copy link
Contributor Author

pantierra commented Feb 6, 2025

This approach should work fine as well. We could define in values.yaml:

apiServices:
  - raster
  - stac
  - vector

and then just check for them in the charts with:

{{- if has $serviceName $.Values.apiServices }}

If I recall correctly, when @ciaransweet and I discussed this a few months ago, we considered this structure but preferred nesting them under externalServices—or better yet, as you suggest, something like eoAPIServices. I'm fine with either. Which do we prefer? Any arguments for or against each option?

@pantierra
Copy link
Contributor Author

pantierra commented Feb 6, 2025

I hope this works for you. I renamed externalServices to apiServices because it's shorter than eoAPIServices, and the latter seems to break the camel case convention with the uppercase "API."

@ividito
Copy link
Contributor

ividito commented Feb 6, 2025

If I recall correctly, when @ciaransweet and I discussed this a few months ago, we considered this structure but preferred nesting them under externalServices—or better yet, as you suggest, something like eoAPIServices. I'm fine with either. Which do we prefer? Any arguments for or against each option?

I would prefer to keep existing values.yml files valid for a little while longer, which leads me away from the nested values. That being said, I don't want to hold things up. It might also be possible to get the same backwards compatibility using a helper function (although that might add more complexity than it's worth).

@pantierra pantierra changed the title Move raster, vector and stac into externalServices. Nest raster, vector and stac services into apiServices. Feb 7, 2025
@pantierra
Copy link
Contributor Author

pantierra commented Feb 7, 2025

Indeed this change needs more evaluation and proper documentation and maybe even assistance to roll it out. Thus i suggest we do a twp-step approach:

@batpad
Copy link
Member

batpad commented Feb 7, 2025

This enhancement enables subcharts to extend these services, facilitating the injection of additional external services, such as an application frontend.

Nothing to block the PR, but just curious: as someone slightly fuzzy with how subcharts work but having encountered similar issues, would you have a link to an example of how this would work with a subchart extending to add a custom frontend, for eg? Linking to some existing subchart docs to demonstrate the idea should be fine - I just wanted to understand a bit clearly how the nesting helps here for the subchart use-case. Thanks!

@pantierra
Copy link
Contributor Author

Good point about adding documentation. We don’t have an example yet, but polder would be a good first use case.

The main idea is that a deployment using eoapi-k8s as a dependency can easily integrate additional services—such as another services API or frontend—via the values.yaml, regardless of whether they are defined as variables (#161) or nested configurations (this PR). These services will then be picked up by templates/service/services.yaml (and others) and deployed within the same context, utilizing the existing ingress.

@pantierra pantierra force-pushed the feature/extend-public-services branch from bfed7d6 to 576f1e7 Compare February 7, 2025 16:11
@pantierra pantierra force-pushed the feature/extend-public-services branch from 576f1e7 to ac4a980 Compare February 7, 2025 16:14
@pantierra
Copy link
Contributor Author

We merged step 1 into main (#180). Now, I’ve merged it back into this branch to focus solely on the nesting aspect.

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