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: emit changed flags in configuration change event #925

Merged
merged 2 commits into from
Sep 16, 2024
Merged

feat: emit changed flags in configuration change event #925

merged 2 commits into from
Sep 16, 2024

Conversation

UtkarshSharma2612
Copy link
Contributor

@UtkarshSharma2612 UtkarshSharma2612 commented Aug 24, 2024 •
edited by toddbaert
Loading

Adds changed flags to in-process change event.

Fixes: #901

@UtkarshSharma2612 UtkarshSharma2612 requested a review from a team as a code owner August 24, 2024 13:38
@UtkarshSharma2612 UtkarshSharma2612 mentioned this pull request Aug 24, 2024
Closed
@UtkarshSharma2612 UtkarshSharma2612 marked this pull request as draft August 25, 2024 06:21
@UtkarshSharma2612 UtkarshSharma2612 marked this pull request as ready for review August 26, 2024 16:27
@UtkarshSharma2612 UtkarshSharma2612 force-pushed the fix/changed-flags-in-event branch from 39bcab6 to 46fbbe5 Compare August 26, 2024 16:31
@beeme1mr beeme1mr changed the title fix:passing changed flags in configuration change event feat(flagd): emit changed flags in configuration change event Aug 26, 2024
@toddbaert toddbaert self-requested a review August 26, 2024 20:00
toddbaert
toddbaert requested changes Aug 26, 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.

#925 (comment)

@toddbaert toddbaert requested a review from DBlanchard88 August 27, 2024 15:44
@toddbaert toddbaert force-pushed the fix/changed-flags-in-event branch 2 times, most recently from d86fbf4 to 2b817c5 Compare September 5, 2024 18:00
@toddbaert
Copy link
Member

@UtkarshSharma2612 I squashed your branch to a single commit and left a comment here.

aepfli
aepfli reviewed Sep 13, 2024
View reviewed changes
Copy link
Member

@aepfli aepfli left a comment

Choose a reason for hiding this comment

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

@UtkarshSharma2612 I think this looks good already; my thoughts for the review are more regarding complexity and maybe unneeded transformations for data, which can increase our footprint. Please have a look and let me know what you think

providers/flagd/src/main/java/dev/openfeature/contrib/providers/flagd/FlagdProvider.java Outdated Show resolved Hide resolved
...n/java/dev/openfeature/contrib/providers/flagd/resolver/process/storage/StorageStateDTO.java Outdated Show resolved Hide resolved
...n/java/dev/openfeature/contrib/providers/flagd/resolver/process/storage/StorageStateDTO.java Outdated Show resolved Hide resolved
@UtkarshSharma2612
Copy link
Contributor Author

@aepfli have implemented changes please help with re-review

aepfli
aepfli approved these changes Sep 16, 2024
View reviewed changes
Copy link
Member

@aepfli aepfli left a comment

Choose a reason for hiding this comment

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

lgtm, i am just thinking that actually a set should be enough for the flag keys, maybe instead of class List for the changedFlags we can even use the interface Collection so we can easier switch between different collection implementations. wdyt about using a Hashset, rather than a list for the changedflags? do we need that safety of no duplicates within the changedflags?

@toddbaert toddbaert force-pushed the fix/changed-flags-in-event branch from beb78f9 to 9519a9e Compare September 16, 2024 15:23
@toddbaert toddbaert force-pushed the fix/changed-flags-in-event branch from eec04d1 to 71a73e6 Compare September 16, 2024 19:24
@toddbaert toddbaert changed the title feat(flagd): emit changed flags in configuration change event feat: emit changed flags in configuration change event Sep 16, 2024
@toddbaert toddbaert force-pushed the fix/changed-flags-in-event branch from 71a73e6 to e506a6b Compare September 16, 2024 19:29
@toddbaert toddbaert self-requested a review September 16, 2024 19:29
@toddbaert toddbaert force-pushed the fix/changed-flags-in-event branch from e506a6b to 1ec024c Compare September 16, 2024 19:35
toddbaert
toddbaert approved these changes Sep 16, 2024
View reviewed changes
Copy link
Member

@toddbaert toddbaert 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.

@UtkarshSharma2612 I squashed all your changes, and made a few small changes in this commit:

  • added the changed flags to RPC mode as well (this was just a couple of lines, with the work you already did it was very easy)
  • added a new assertion in the e2e test suite which was previously not implemented because we didn't support flagsChanged
  • some minor checkstyle and spotbugs fixes

Really great work! Thanks for doing all the heavy lifting here!

@toddbaert toddbaert force-pushed the fix/changed-flags-in-event branch from 1ec024c to 067129f Compare September 16, 2024 19:39
@toddbaert toddbaert force-pushed the fix/changed-flags-in-event branch from 067129f to ac5896c Compare September 16, 2024 19:43
@toddbaert toddbaert merged commit d3de874 into open-feature:main Sep 16, 2024
4 checks passed
@github-actions github-actions bot mentioned this pull request Sep 14, 2024
Merged
@UtkarshSharma2612
Copy link
Contributor Author

UtkarshSharma2612 commented Sep 17, 2024 •
edited
Loading

@UtkarshSharma2612 I squashed all your changes, and made a few small changes in this commit:

  • added the changed flags to RPC mode as well (this was just a couple of lines, with the work you already did it was very easy)
  • added a new assertion in the e2e test suite which was previously not implemented because we didn't support flagsChanged
  • some minor checkstyle and spotbugs fixes

Really great work! Thanks for doing all the heavy lifting here!

Thank you, @toddbaert! I really enjoyed working on this issue and learned a lot in the process. Are there any other open issues I could contribute to? I'd love to continue helping out. 😄

@toddbaert
Copy link
Member

toddbaert commented Sep 18, 2024 •
edited
Loading

@UtkarshSharma2612 I squashed all your changes, and made a few small changes in this commit:

  • added the changed flags to RPC mode as well (this was just a couple of lines, with the work you already did it was very easy)
  • added a new assertion in the e2e test suite which was previously not implemented because we didn't support flagsChanged
  • some minor checkstyle and spotbugs fixes

Really great work! Thanks for doing all the heavy lifting here!

Thank you, @toddbaert! I really enjoyed working on this issue and learned a lot in the process. Are there any other open issues I could contribute to? I'd love to continue helping out. 😄

@UtkarshSharma2612 I'm sure there is. I'll let you know.

I will also invite you to the org, which is the first step to getting more involved.

@toddbaert toddbaert mentioned this pull request Sep 18, 2024
Merged
@UtkarshSharma2612
Copy link
Contributor Author

@UtkarshSharma2612 I squashed all your changes, and made a few small changes in this commit:

  • added the changed flags to RPC mode as well (this was just a couple of lines, with the work you already did it was very easy)
  • added a new assertion in the e2e test suite which was previously not implemented because we didn't support flagsChanged
  • some minor checkstyle and spotbugs fixes

Really great work! Thanks for doing all the heavy lifting here!

Thank you, @toddbaert! I really enjoyed working on this issue and learned a lot in the process. Are there any other open issues I could contribute to? I'd love to continue helping out. 😄

@UtkarshSharma2612 I'm sure there is. I'll let you know.

I will also invite you to the org, which is the first step to getting more involved.

Sure, Thanks!

@aepfli
Copy link
Member

aepfli commented Sep 19, 2024

Hey @UtkarshSharma2612, it was also a pleasure working with you.

As you seem to be interested, today I opened #957, which would be a cool improvement for our testing tooling. ;)

@aepfli aepfli mentioned this pull request Sep 25, 2024
Merged
@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

@beeme1mr beeme1mr Awaiting requested review from beeme1mr

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

@thisthat thisthat Awaiting requested review from thisthat

@DBlanchard88 DBlanchard88 Awaiting requested review from DBlanchard88

Assignees

@beeme1mr beeme1mr

@thisthat thisthat

@Kavindu-Dodan Kavindu-Dodan

@toddbaert toddbaert

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[flagd-in-process] Support changed flag keys in the provider configuration change event
6 participants
@UtkarshSharma2612 @toddbaert @aepfli @beeme1mr @thisthat @Kavindu-Dodan