-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 #1725
Add flagd UI #1725
Conversation
1174044
to
a07108d
Compare
Hey @maindotmarcell, I'm Mike from the OpenFeature team. Thanks for getting this started! Please let me know if you need our support. By the way, I've been experimenting with a flagd compatible rule builder React component. Perhaps I could contribute to that once the initial version is live. |
Hi @beeme1mr, sounds awesome! Thanks for letting me know, we could definitely add something like a rule builder to this eventually :) |
7fb7834
to
e33d518
Compare
Hi @maindotmarcell, there are some failing checks related to markdown-lint, sanity, and license. Could you fix those so we can proceed with the PR? |
299a480
to
b38638e
Compare
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.
Thanks for adding this, that's such a nice new feature!
I cannot provide my review from the code perspective (nextjs), but from the microservice project structure it looks good overall.
Could you add the microservice in the .github/workflow
files too? This is needed for the image generation. You can find some examples in https://github.com/open-telemetry/opentelemetry-demo/blob/main/.github/workflows/component-build-images.yml#L38
Feel free to ping me on CNCF's Slack if you need any help/guidance with the CI part
@rogercoll Thank you for the feedback! Good point, I'll add the service in the workflows :) |
b38638e
to
6f48cc4
Compare
@rogercoll I added the changes to the workflow, could you please check if this is correct please? 🙏 |
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.
Code structure lgtm, please @open-telemetry/demo-approvers could you help to review the Typescript part?
Thanks!
6f48cc4
to
e0d60b2
Compare
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.
Hello @maindotmarcell 👋🏽
Thank you for this PR, the /feature
is something a lot of users wanted back.
I've left a couple of comments in your PR, mainly asking you to keep the changes to the flagd-ui service only, this makes easier to review further and keep track of what you are actually adding.
One main thing that I've noticed is that you are only using the @vercel/otel
instrumentation, so I've left a suggestion to get rid of all other OTel dependencies.
And last but not least, if you could change the exporter http
to grpc
that would make the .env
file nicer.
Ah, one last thing, remember to update the package-lock.json
file whenever updating the deps. Saying because I've already missed that some time ago 😊
e0d60b2
to
58de2a9
Compare
@julianocosta89 I've addressed all the changes you requested. Applied everything with the exception of changing otel exporter from http to grpc as for now I could only get it working with http. Let me know if you have any additional remarks :) |
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.
Hey @maindotmarcell thanks for taking care of the comments.
I've added a couple of minor ones.
Also are you able to merge main into your PR and take a look at the merge conflicts?
I saw that in the docker-compose.yml
file you have some changes that are not from your PR, I believe merging main here will solve that.
44b9f83
to
a33d686
Compare
a33d686
to
7e2b1a0
Compare
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.
Great work @maindotmarcell!
Thanks for your patience while addressing all comments.
@maindotmarcell would you be able to resolve the |
7e2b1a0
to
1d9ca01
Compare
@julianocosta89 Awesome! just did it :) |
cd6e901
to
93b7c78
Compare
93b7c78
to
d6f2639
Compare
Changes
We're adding a user interface to the demo, through which users of the demo can update the Flagd feature flag configuration file in a more user friendly way.
Finished changes
Suggested roadmap for future contributions
Merge Requirements
For new features contributions, please make sure you have completed the following
essential items:
CHANGELOG.md
updated to document new feature additionsMaintainers 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.