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

load product catalog periodically #1919

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

Conversation

puckpuck
Copy link
Contributor

@puckpuck puckpuck commented Jan 16, 2025

Changes

Fixes #1892

  • adds a new environment variable called: PRODUCT_CATALOG_RELOAD_INTERVAL
  • reloads the product catalog according to an interval based on the env variable (defaults to 10 seconds)
  • mounts the product.json file at run time instead of including it at image build time

This allows the user to change the products without rebuilding the product-catalog service. The products can also be updated after the demo is running. The change also paves the way for reloading the product catalog on a period timer, allowing the product list to be backed by more dynamic sources such as a database.

Finally, this change will require some updates to the Helm chart to load and mount the product list via a ConfigMap.

Merge Requirements

For new features contributions, please make sure you have completed the following
essential items:

  • CHANGELOG.md updated to document new feature additions
  • Appropriate documentation updates in the docs
  • Appropriate Helm chart updates in the helm-charts

Maintainers will not merge until the above have been completed. If you're unsure
which docs need to be changed ping the
@open-telemetry/demo-approvers.

@puckpuck puckpuck requested a review from a team as a code owner January 16, 2025 05:19
@github-actions github-actions bot added the helm-update-required Requires an update to the Helm chart when released label Jan 16, 2025
Copy link
Contributor

@rogercoll rogercoll left a comment

Choose a reason for hiding this comment

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

lgtm overall

@@ -26,7 +26,7 @@
},
{
"id": "1YMWWN1N4O",
"name": "Eclipsmart Travel Refractor Telescope",
"name": "THIS IS MY PRODUCT NAME",
Copy link
Contributor

Choose a reason for hiding this comment

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

Left over?

ticker := time.NewTicker(time.Duration(interval) * time.Second)

go func() {
for {
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be best to have the programs' context here so it can be properly shutdown, but it seems it would require a major refactor to the codebase (all other init functions as well). We can keep it for a future improvement.

@@ -399,6 +399,7 @@ services:
environment:
- FLAGD_HOST
- PRODUCT_CATALOG_PORT
- PRODUCT_CATALOG_RELOAD_INTERVAL
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we mount the products to the container?

Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Jan 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
helm-update-required Requires an update to the Helm chart when released Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

reload Product catalog periodically
2 participants