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

fix: max System.Collections.Immutable version ++ #137

Merged
merged 1 commit into from
Jul 14, 2023

Conversation

toddbaert
Copy link
Member

@toddbaert toddbaert commented Jul 14, 2023

remove max System.Collections.Immutable version

fixes: #136

@toddbaert toddbaert requested a review from a team as a code owner July 14, 2023 17:55
@codecov
Copy link

codecov bot commented Jul 14, 2023

Codecov Report

Merging #137 (46fa6f2) into main (16f3300) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #137   +/-   ##
=======================================
  Coverage   94.64%   94.64%           
=======================================
  Files          20       20           
  Lines         542      542           
  Branches       39       39           
=======================================
  Hits          513      513           
  Misses         16       16           
  Partials       13       13           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@kinyoklion
Copy link
Member

Generally speaking this is a little concerning, because MS could change something and cause unpredictable behavior. On the other hand they have given up on the version having much meaning, so maybe this is the only alternative.

@toddbaert
Copy link
Member Author

Generally speaking this is a little concerning, because MS could change something and cause unpredictable behavior. On the other hand they have given up on the version having much meaning, so maybe this is the only alternative.

I was concerned about this too... but I think if there was a breaking change the impacted us, we would see it relatively quickly in the CI, as long is it was in .NET 6 or 7.. Maybe I'm wrong about that.

@kinyoklion
Copy link
Member

Generally speaking this is a little concerning, because MS could change something and cause unpredictable behavior. On the other hand they have given up on the version having much meaning, so maybe this is the only alternative.

I was concerned about this too... but I think if there was a breaking change the impacted us, we would see it relatively quickly in the CI, as long is it was in .NET 6 or 7.. Maybe I'm wrong about that.

I am sure we would detect it fast, but would consumer applications basically be breaking at the same time? Because they nuget install and different packages are fetched?

@kinyoklion
Copy link
Member

Generally speaking dotnet tries to install the lowest compatible version (backward from most package systems), at least it used to, so it may not be a problem.

@toddbaert
Copy link
Member Author

toddbaert commented Jul 14, 2023

Generally speaking dotnet tries to install the lowest compatible version (backward from most package systems), at least it used to, so it may not be a problem.

I just found this out, yes. Locally this is causing 1.7.1 to be resolved, which doesn't really help in testing.

I just manually forced version 7 and ran the unit and e2e test suite and that worked... so I guess the more conservative option would be to allow up to v8 (exclusive).

Any thoughts?

@kinyoklion
Copy link
Member

Generally speaking a dotnet application will actually automatically generate binding redirects allowing you to use any version. ASP.Net applications do not, which is generally where this problem comes up.

For instance, for LD, this comes up with Microsoft.Logging.Extensions all the time:

<dependentAssembly>
  <assemblyIdentity name="Microsoft.Extensions.Logging"
    publicKeyToken="adb9793829ddae60"
    culture="en-us" />
  <bindingRedirect oldVersion="6.0.0" newVersion="7.0.0.0" />
</dependentAssembly>

Updating it to 8.0.0 is safest. But it does mean it needs basically annually updated. Because MS will release the next one.

@toddbaert
Copy link
Member Author

Generally speaking a dotnet application will actually automatically generate binding redirects allowing you to use any version. ASP.Net applications do not, which is generally where this problem comes up.

For instance, for LD, this comes up with Microsoft.Logging.Extensions all the time:

<dependentAssembly>
  <assemblyIdentity name="Microsoft.Extensions.Logging"
    publicKeyToken="adb9793829ddae60"
    culture="en-us" />
  <bindingRedirect oldVersion="6.0.0" newVersion="7.0.0.0" />
</dependentAssembly>

Updating it to 8.0.0 is safest. But it does mean it needs basically annually updated. Because MS will release the next one.

OK I'll do the conservative thing for now and then think on it more. Maybe we can automate it somehow.

Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
@kinyoklion
Copy link
Member

May want to update the PR title, for the release note.

@toddbaert toddbaert changed the title fix: remove max System.Collections.Immutable v fix: max System.Collections.Immutable version ++ Jul 14, 2023
@toddbaert toddbaert merged commit 55c5e8e into main Jul 14, 2023
11 checks passed
@toddbaert toddbaert deleted the fix/no-max-immutable-version branch July 14, 2023 19:01
austindrenski added a commit to austindrenski/open-feature-dotnet-sdk that referenced this pull request Jan 11, 2024
austindrenski added a commit to austindrenski/open-feature-dotnet-sdk that referenced this pull request Jan 11, 2024
austindrenski added a commit to austindrenski/open-feature-dotnet-sdk that referenced this pull request Jan 11, 2024
austindrenski added a commit to austindrenski/open-feature-dotnet-sdk that referenced this pull request Jan 16, 2024
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.

System.Collections.Immutable needs to be updated to allow for version 7
3 participants