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.

Sign up for GitHub

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

Jump to bottom

feat: [flagd] Default port to 8015 if in-process resolver is used. #810

Merged
merged 2 commits into from
Jun 11, 2024
Merged

feat: [flagd] Default port to 8015 if in-process resolver is used. #810

merged 2 commits into from
Jun 11, 2024

Conversation

zedadyna
Copy link
Contributor

@zedadyna zedadyna commented Jun 7, 2024

closes: #809

This PR

The flagd provider defaults to RPC mode and the corresponding port (8013) using the evaluation proto. If the in-process resolver is selected, it operates in in-process mode using the sync proto, but still uses port 8013, instead of defaulting to the correct port (8015, for the sync proto).

We improve the configuration by defaulting to port 8015 if the in-process resolver is selected.

Related Issues

Fixes #809

@zedadyna zedadyna requested a review from a team as a code owner June 7, 2024 09:07
@zedadyna zedadyna force-pushed the ISSUE-809/default-port branch 2 times, most recently from 5f2a504 to 4f693de Compare June 7, 2024 09:14
thisthat
thisthat approved these changes Jun 7, 2024
View reviewed changes
Copy link
Member

@thisthat thisthat left a comment

Choose a reason for hiding this comment

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

Overall, LGTM :shipit:
Thanks @zedadyna for your contribution. Before merging, I think we should address the discussion on how to test env var and if we should move this to a dedicated ticket 🤔

providers/flagd/src/test/java/dev/openfeature/contrib/providers/flagd/FlagdOptionsTest.java Show resolved Hide resolved
Kavindu-Dodan
Kavindu-Dodan approved these changes Jun 7, 2024
View reviewed changes
Copy link
Collaborator

@Kavindu-Dodan Kavindu-Dodan left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thank you @zedadyna for working on this. I left few nits, othwerwise this is a good improvement :)

Ohh and the build fails because of checkstyle [1]. You can run and validate this locallly with mvn org.apache.maven.plugins:maven-checkstyle-plugin:3.4.0:check

[1] - https://github.com/open-feature/java-sdk-contrib/actions/runs/9414628756/job/25957413948?pr=810#step:6:4785

@Kavindu-Dodan Kavindu-Dodan self-requested a review June 7, 2024 19:38
@toddbaert
Copy link
Member

Yes @zedadyna the build is failing because you're missing some javadoc:

Error: /home/runner/work/java-sdk-contrib/java-sdk-contrib/providers/flagd/src/main/java/dev/openfeature/contrib/providers/flagd/FlagdOptions.java:108:5: Missing a Javadoc comment. [MissingJavadocMethod]
Error: /home/runner/work/java-sdk-contrib/java-sdk-contrib/providers/flagd/src/main/java/dev/openfeature/contrib/providers/flagd/Config.java:15: Summary javadoc is missing. [SummaryJavadoc]

toddbaert
toddbaert approved these changes Jun 10, 2024
View reviewed changes
Copy link
Member

@toddbaert toddbaert left a comment

Choose a reason for hiding this comment

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

Agree with some of the minor feedback in here. If we want to discuss env testing a bit more, that can be in a new issue.

Approving, but you'll have to fix the CI issue first.

@zedadyna zedadyna force-pushed the ISSUE-809/default-port branch from b47731d to 6eaa321 Compare June 11, 2024 05:32
@zedadyna zedadyna force-pushed the ISSUE-809/default-port branch from 6eaa321 to 1125869 Compare June 11, 2024 05:34
zedadyna and others added 2 commits June 11, 2024 07:34
@zedadyna @alexandraoberaigner
feat: [flagd] Default port to 8015 if in-process resolver is used.
7a4b3fc 
closes: open-feature#809
Signed-off-by: Daniel Zehetner <daniel.zehetner@dynatrace.com>
Co-authored-by: Alexandra Oberaigner <alexandra.oberaigner@dynatrace.com>
Signed-off-by: Daniel Zehetner <daniel.zehetner@dynatrace.com>
@zedadyna
Fixed checkstyle violations and some minor improvements out of PR com…
72e9bf6 
…ments

Signed-off-by: Daniel Zehetner <daniel.zehetner@dynatrace.com>
@zedadyna zedadyna force-pushed the ISSUE-809/default-port branch from 1125869 to 72e9bf6 Compare June 11, 2024 05:34
@aepfli aepfli mentioned this pull request Jun 11, 2024
Closed
@aepfli aepfli mentioned this pull request Jun 11, 2024
Closed
@aepfli
Copy link
Member

aepfli commented Jun 11, 2024

Agree with some of the minor feedback in here. If we want to discuss env testing a bit more, that can be in a new issue.

Approving, but you'll have to fix the CI issue first.

created #818 to discuss the env testing topic

@Kavindu-Dodan Kavindu-Dodan merged commit 9b7dc9a into open-feature:main Jun 11, 2024
4 checks passed
@github-actions github-actions bot mentioned this pull request Jun 11, 2024
Merged
@zedadyna zedadyna deleted the ISSUE-809/default-port branch June 12, 2024 06:09
@github-actions github-actions bot mentioned this pull request Jan 3, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@toddbaert toddbaert toddbaert approved these changes

@aepfli aepfli aepfli approved these changes

@thisthat thisthat thisthat approved these changes

@beeme1mr beeme1mr Awaiting requested review from beeme1mr

@Kavindu-Dodan Kavindu-Dodan Awaiting requested review from Kavindu-Dodan

Assignees

@beeme1mr beeme1mr

@thisthat thisthat

@thiyagu06 thiyagu06

@Kavindu-Dodan Kavindu-Dodan

@thomaspoignant thomaspoignant

@toddbaert toddbaert

@laliconfigcat laliconfigcat

@liran2000 liran2000

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[flagd] Default port to 8015 if in-process resolver is used.
10 participants
@zedadyna @toddbaert @aepfli @thisthat @Kavindu-Dodan @beeme1mr @thiyagu06 @thomaspoignant @laliconfigcat @liran2000