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 Flagd-ui to demo docs #5332

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

Conversation

maindotmarcell
Copy link

@maindotmarcell maindotmarcell commented Oct 7, 2024

Changes

Changes required for the documentation concerning the flagd-ui feature.

As we have a ready PR for the first version of the flagd-ui, we would like to start thinking about how this new feature can be included in the documentation. Below you can find what I've implemented in this PR as my suggestions:

  • Screenshots -> Add screenshots of the UI
  • Docker deployment -> Add :8080/feature route
  • Kubernetes deployment -> Add :8080/feature route
  • Feature flags page -> Mention the possibility to toggle the flags through the UI
  • Requirements/architecture -> mention under feature flagging section

As it is not directly a microservice of the astronomy shop app, but it is an instrumented service nonetheless, there a few where it might make sense to include it (the flagd service itself is currently not included in these):

  • Architecture charts
  • List of Services

Additionally it might make sense to create a separate docs page for this feature at some point.

Thank you in advance for reviewing! 🚀

Let me know if I missed anything :)

@opentelemetrybot opentelemetrybot requested a review from a team October 7, 2024 09:48
Copy link
Member

@svrnm svrnm left a comment

Choose a reason for hiding this comment

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

Thank you, I commented with a few change requests

content/en/docs/demo/feature-flags.md Outdated Show resolved Hide resolved
content/en/docs/demo/feature-flags.md Outdated Show resolved Hide resolved
content/en/docs/demo/feature-flags.md Outdated Show resolved Hide resolved
content/en/docs/demo/feature-flags.md Outdated Show resolved Hide resolved
content/en/docs/demo/requirements/architecture.md Outdated Show resolved Hide resolved
@maindotmarcell
Copy link
Author

Thank you, I commented with a few change requests

@svrnm Thank you for the review, I've applied the suggested changes in the last commit

@svrnm
Copy link
Member

svrnm commented Oct 9, 2024

Thank you, I commented with a few change requests

@svrnm Thank you for the review, I've applied the suggested changes in the last commit

thanks!

@open-telemetry/demo-approvers please take a look!

Copy link
Member

@julianocosta89 julianocosta89 left a comment

Choose a reason for hiding this comment

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

LGTM, we just need to get the Helm chart PR merged before merging this one to avoid user's confusion.

Also, could you add the flagd-ui to the architecture page?
https://github.com/open-telemetry/opentelemetry.io/blob/main/content/en/docs/demo/architecture.md

I have it updated here if you would like: mermaid.live

content/en/docs/demo/feature-flags.md Outdated Show resolved Hide resolved
@opentelemetrybot opentelemetrybot requested a review from a team October 16, 2024 08:51
Copy link
Member

@julianocosta89 julianocosta89 left a comment

Choose a reason for hiding this comment

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

One minor suggestion, renaming the title and moving the initial sentence.

content/en/docs/demo/feature-flags.md Outdated Show resolved Hide resolved
content/en/docs/demo/feature-flags.md Show resolved Hide resolved
content/en/docs/demo/feature-flags.md Outdated Show resolved Hide resolved
@julianocosta89
Copy link
Member

LGTM, just waiting on open-telemetry/opentelemetry-helm-charts#1367

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants