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

chore: revert breaking setProvider #190

Merged
merged 22 commits into from
Jan 23, 2024

Conversation

toddbaert
Copy link
Member

This PR makes the breaking change here non breaking, by adding a new method instead. This means our pending release will no longer be breaking.

This will allow us to release all the pending .NET work, without merging #184. I like the idea of #184, but I think I'd like us all to take some time to discuss exactly what we expect the cancellationToken to do in all circumstances (what should the SDK do on SDK methods like init()/shutdown()?, and what should providers do?).

I think buys us some without sacrificing too much, and we can release a 2.0.0 in the coming weeks.

Interested to hear the opinion of others.

@toddbaert toddbaert requested a review from a team as a code owner January 17, 2024 21:08
@toddbaert toddbaert marked this pull request as draft January 17, 2024 21:08
Copy link

codecov bot commented Jan 17, 2024

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (26cd5cd) 93.86% compared to head (528b723) 93.50%.

❗ Current head 528b723 differs from pull request most recent head a9d9fbc. Consider uploading reports for the commit a9d9fbc to get more accurate results

Files Patch % Lines
src/OpenFeature/Api.cs 50.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #190      +/-   ##
==========================================
- Coverage   93.86%   93.50%   -0.37%     
==========================================
  Files          23       23              
  Lines         946      954       +8     
  Branches      105      105              
==========================================
+ Hits          888      892       +4     
- Misses         34       38       +4     
  Partials       24       24              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@austindrenski austindrenski left a comment

Choose a reason for hiding this comment

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

Nice, should provide a gentler migration path downstream.

Couple of suggestions, including a while-we're-at-it related to #185 🤞

src/OpenFeature/Api.cs Outdated Show resolved Hide resolved
src/OpenFeature/Api.cs Outdated Show resolved Hide resolved
src/OpenFeature/Api.cs Show resolved Hide resolved
src/OpenFeature/Api.cs Show resolved Hide resolved
src/OpenFeature/Api.cs Show resolved Hide resolved
src/OpenFeature/Api.cs Show resolved Hide resolved
src/OpenFeature/Api.cs Show resolved Hide resolved
test/OpenFeature.Tests/OpenFeatureTests.cs Outdated Show resolved Hide resolved
@lukas-reining
Copy link
Member

I like the idea of #184, but I think I'd like us all to take some time to discuss exactly what we expect the cancellationToken to do in all circumstances (what should the SDK do on SDK methods like init()/shutdown()?, and what should providers do?).

I like the idea @toddbaert, and I think we could also add them later without even having a breaking change.
I think especially for getting flags we should later add cancellationTokens but maybe as you said with first discussing all the possible side effects.

@toddbaert
Copy link
Member Author

toddbaert commented Jan 18, 2024

@austindrenski I've accepted a few suggestions, and noted that others, which I did not accept are corollaries to this discussion and don't apply if we go with the route proposed there. Please re-review.

Based on the slack discussion, it sounds like we'd like to go the route of merging this to release a non-breaking version, and then subsequently working on a breaking release with a bunch of async changes, including moving some methods over to ValueTask as you proposed.

@lukas-reining @askpt @kinyoklion @benjiro @bacherfl

@toddbaert toddbaert marked this pull request as ready for review January 18, 2024 18:52
@toddbaert toddbaert force-pushed the chore/revert-breaking-setProvider-change branch from cdc5153 to 1ae7b1c Compare January 18, 2024 19:04
Copy link
Member

@austindrenski austindrenski left a comment

Choose a reason for hiding this comment

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

LGTM 🚢🚢🚢🚢

@austindrenski
Copy link
Member

@toddbaert wrote (#190 (comment)):

Based on the slack discussion, it sounds like we'd like to go the route of merging this to release a non-breaking version, and then subsequently working on a breaking release with a bunch of async changes, including moving some methods over to ValueTask as you proposed.

Sounds good, but would like to flag #186 for consideration before we cut the non-breaking release.

I opened #188 with a proposed solution, but want to call out that I drafted it with the assumption that the next release would be a breaking release, so it would help to get some fresh eyes on it.

The downside to not fixing #186 before the next release is that shutdown/restart will remain broken, but since its already broken, it's not a showstopper if we need to punt.

Copy link
Member

@askpt askpt left a comment

Choose a reason for hiding this comment

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

LGTM! Unfortunately, our test coverage decreased due to the new methods being introduced.

@toddbaert
Copy link
Member Author

@toddbaert wrote (#190 (comment)):

Based on the slack discussion, it sounds like we'd like to go the route of merging this to release a non-breaking version, and then subsequently working on a breaking release with a bunch of async changes, including moving some methods over to ValueTask as you proposed.

Sounds good, but would like to flag #186 for consideration before we cut the non-breaking release.

I opened #188 with a proposed solution, but want to call out that I drafted it with the assumption that the next release would be a breaking release, so it would help to get some fresh eyes on it.

The downside to not fixing #186 before the next release is that shutdown/restart will remain broken, but since its already broken, it's not a showstopper if we need to punt.

I will take a look, this is the sort of thing I'd like to have resolved in this release. 🙏

@toddbaert toddbaert force-pushed the chore/revert-breaking-setProvider-change branch from ca3ac87 to c6657f6 Compare January 19, 2024 15:54
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
@toddbaert toddbaert force-pushed the chore/revert-breaking-setProvider-change branch from c6657f6 to d34331a Compare January 19, 2024 17:23
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
test/OpenFeature.Tests/OpenFeatureEventTests.cs Outdated Show resolved Hide resolved
test/OpenFeature.Tests/OpenFeatureTests.cs Outdated Show resolved Hide resolved
test/OpenFeature.Tests/OpenFeatureTests.cs Outdated Show resolved Hide resolved
test/OpenFeature.Tests/OpenFeatureTests.cs Outdated Show resolved Hide resolved
test/OpenFeature.Tests/OpenFeatureTests.cs Outdated Show resolved Hide resolved
test/OpenFeature.Tests/OpenFeatureTests.cs Outdated Show resolved Hide resolved
test/OpenFeature.Tests/ProviderRepositoryTests.cs Outdated Show resolved Hide resolved
test/OpenFeature.Tests/ProviderRepositoryTests.cs Outdated Show resolved Hide resolved
test/OpenFeature.Tests/ProviderRepositoryTests.cs Outdated Show resolved Hide resolved
toddbaert and others added 9 commits January 22, 2024 10:13
Co-authored-by: Austin Drenski <austin@austindrenski.io>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Co-authored-by: Austin Drenski <austin@austindrenski.io>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Co-authored-by: Austin Drenski <austin@austindrenski.io>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Co-authored-by: Austin Drenski <austin@austindrenski.io>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Co-authored-by: Austin Drenski <austin@austindrenski.io>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Co-authored-by: Austin Drenski <austin@austindrenski.io>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Co-authored-by: Austin Drenski <austin@austindrenski.io>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Co-authored-by: Austin Drenski <austin@austindrenski.io>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Co-authored-by: Austin Drenski <austin@austindrenski.io>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
@toddbaert
Copy link
Member Author

There was some fun pseudo-conflicts on this branch after recent merges, but I think we should be good now.

Copy link
Member

@austindrenski austindrenski left a comment

Choose a reason for hiding this comment

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

Little follow-up cleanup because GitHub got weird applying those suggestions

test/OpenFeature.Tests/OpenFeatureTests.cs Outdated Show resolved Hide resolved
test/OpenFeature.Tests/OpenFeatureTests.cs Outdated Show resolved Hide resolved
test/OpenFeature.Tests/OpenFeatureTests.cs Outdated Show resolved Hide resolved
test/OpenFeature.Tests/OpenFeatureTests.cs Outdated Show resolved Hide resolved
toddbaert and others added 4 commits January 22, 2024 10:22
Co-authored-by: Austin Drenski <austin@austindrenski.io>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Co-authored-by: Austin Drenski <austin@austindrenski.io>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Co-authored-by: Austin Drenski <austin@austindrenski.io>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Co-authored-by: Austin Drenski <austin@austindrenski.io>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Copy link
Member

@austindrenski austindrenski left a comment

Choose a reason for hiding this comment

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

One more cleanup because the UI is clearly having some issues this morning, but otherwise LGTM (again)

Co-authored-by: Austin Drenski <austin@austindrenski.io>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
@toddbaert
Copy link
Member Author

One more cleanup because the UI is clearly having some issues this morning, but otherwise LGTM (again)

At least we can be reasonably satisfied the tests are stable now, they've run like 30 times this morning due to this PR 😂

@toddbaert toddbaert merged commit 2919c2f into main Jan 23, 2024
10 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants