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

update .net core 3.0 RTM #1025

Merged
merged 44 commits into from
Oct 28, 2019
Merged

update .net core 3.0 RTM #1025

merged 44 commits into from
Oct 28, 2019

Conversation

geffzhang
Copy link
Contributor

@geffzhang geffzhang commented Sep 24, 2019

Fixes / New Feature #
Fix #1001 and update .net core 3.0 RTM

@catcherwong
Copy link
Contributor

The .NET SDK version of both continuous-integrations is 2.2.x.

Maybe we should update the configuration of CI as well.

mono 6.0.0 and dotnet 3.0.100
update Microsoft.Data.SQLite 3.0.0
.travis.yml Outdated
@@ -11,9 +11,9 @@ dist: bionic
osx_image: xcode9.2
Copy link
Contributor

Choose a reason for hiding this comment

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

The value of osx_image should update to xcode9.4 or above.

<PackageReference Include="IdentityServer4" Version="3.0.1" />
</ItemGroup>
<ItemGroup>
<PackageReference Update="Microsoft.SourceLink.GitHub" Version="1.0.0-beta2-19367-01" />
Copy link
Contributor

@Varorbc Varorbc Sep 27, 2019

Choose a reason for hiding this comment

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

The update of this package should be here

<PackageReference Include="Microsoft.SourceLink.GitHub" Version="1.0.0-beta2-18618-05" PrivateAssets="All"/>

@dennismdejong
Copy link

Please retry the AppVeyor build as .Net Core 3.0 is available right now and wasen't at the moment the build was queued

@jmezach
Copy link
Contributor

jmezach commented Oct 18, 2019

@Leandropintogit Interesting discussion which I think demonstrates the exact behaviour I'm seeing as well. Doesn't look like it's likely to be changed back though, so I think Ocelot will need to react to this change.

As for my changes, I think they actually make sense. With the original implementation it seems that the ConsulFileConfigurationRepository is trying to use the IInternalConfiguration that it is itself responsible for retrieving from Consul. That basically leads to a bit of a chicken and egg problem. With my changes the ConsulFileConfigurationRepository relies on a piece of statically available configuration which isn't very likely to change once Ocelot has started. In fact, that is exactly what the documentation suggest you do; set the service discovery provider information in ocelot.json and the rest of the configuration will be read from there.

Still interested to hear what @TomPallister and @thiagoloureiro think about my change though ;).

@Leandropintogit
Copy link

After digging in some more I think I have a fairly simple solution to this issue, but I'm not entirely sure that it's the correct way to fix the issue. It works fine for my use case though. See my commit jmezach@a0ff389 to see what I've changed @thiagoloureiro @TomPallister

@jmezach
Good job. I just changed the order of assignment of the _configurationKey variable

var serviceDiscoveryProvider = fileConfiguration.Value.GlobalConfiguration.ServiceDiscoveryProvider;
_configurationKey = serviceDiscoveryProvider.ConfigurationKey ?? "InternalConfiguration";

        `var config = new ConsulRegistryConfiguration(serviceDiscoveryProvider.Host,
            serviceDiscoveryProvider.Port, _configurationKey, serviceDiscoveryProvider.Token);`

        `_consul = factory.Get(config);`

@Hbib-drif
Copy link

Great work guys , is there an update or the issue is still not fixed ?

@jesnasali
Copy link

Is there an update?

@jmezach
Copy link
Contributor

jmezach commented Oct 21, 2019

I made another pull request (geffzhang/Ocelot#11) with my proposed fix as described earlier, including the fix in ordering of variable assignment that @Leandropintogit mentioned. Let me know if you want to take it ;).

Fix startup issue with Consul
@jmezach
Copy link
Contributor

jmezach commented Oct 21, 2019

Whoops, forgot to fix the tests it seems. Will fix that ;).

@pranavpatil19
Copy link

is there an issue or the issue is still not fixed ?

@jmezach
Copy link
Contributor

jmezach commented Oct 21, 2019

@pranavpatil19 Well, if you're not using Consul as your configuration store there's no issue, at least not that I'm aware off. If you are, then you'll want to hold off on moving to .NET Core 3.0 for now ;).

@LGinC
Copy link

LGinC commented Oct 22, 2019

I need a new nuget package to fix json issue

@jmezach
Copy link
Contributor

jmezach commented Oct 23, 2019

@geffzhang I made another PR (geffzhang/Ocelot#12) with the fix for the broken tests.

@geffzhang
Copy link
Contributor Author

@jmezach I've merged your pr, the error: [/home/travis/build/ThreeMammals/Ocelot/test/Ocelot.UnitTests/Ocelot.UnitTests.csproj]
Consul/ConsulFileConfigurationRepositoryTests.cs(58,59): error CS1503: Argument 1: cannot convert from 'Ocelot.Cache.IOcelotCache<Ocelot.Configuration.File.FileConfiguration>' to 'Microsoft.Extensions.Options.IOptions<Ocelot.Configuration.File.FileConfiguration>' [/home/travis/build/ThreeMammals/Ocelot/test/Ocelot.UnitTests/Ocelot.UnitTests.csproj]
Consul/ConsulFileConfigurationRepositoryTests.cs(58,74): error CS1503: Argument 2: cannot convert from 'Ocelot.Configuration.Repository.IInternalConfigurationRepository' to 'Ocelot.Cache.IOcelotCache<Ocelot.Configuration.File.FileConfiguration>' [/home/travis/build/ThreeMammals/Ocelot/test/Ocelot.UnitTests/Ocelot.UnitTests.csproj]
Consul/ConsulFileConfigurationRepositoryTests.cs(146,59): error CS1503: Argument 1: cannot convert from 'Ocelot.Cache.IOcelotCache<Ocelot.Configuration.File.FileConfiguration>' to 'Microsoft.Extensions.Options.IOptions<Ocelot.Configuration.File.FileConfiguration>' [/home/travis/build/ThreeMammals/Ocelot/test/Ocelot.UnitTests/Ocelot.UnitTests.csproj]
Consul/ConsulFileConfigurationRepositoryTests.cs(146,74): error CS1503: Argument 2: cannot convert from 'Ocelot.Configuration.Repository.IInternalConfigurationRepository' to 'Ocelot.Cache.IOcelotCache<Ocelot.Configuration.File.FileConfiguration>' [/home/travis/build/ThreeMammals/Ocelot/test/Ocelot.UnitTests/Ocelot.UnitTests.csproj]
1666 Warning(s)

@jmezach
Copy link
Contributor

jmezach commented Oct 23, 2019

Turned out there were some acceptance tests in there as well which were failing because of a condition I missed. Thank god for tests ;). I created yet another PR (geffzhang#13) which should fix those tests as well.

@geffzhang Not sure where you got that error from. I'm guessing you saw older build results or something. I've seen that happen before as well.

@geffzhang
Copy link
Contributor Author

@thiagoloureiro Release a new version of the nuget package

@pranavpatil19
Copy link

pranavpatil19 commented Oct 26, 2019

@jmezach @geffzhang is there an issue ? waiting for new stable release

@thangchung
Copy link

@thiagoloureiro Release a new version of the nuget package

We're still waiting for this release for a long time :(

@jmezach
Copy link
Contributor

jmezach commented Oct 26, 2019

@pranavpatil19 No issues as far as I know, but we're not able to get this merged. I think we need someone from ThreeMammals to merge this, which seems to include @AbolfazlRajabpour, @anktsrkr, @binarymash, @briansantura, @jps, @philproctor, @thiagoloureiro and @TomPallister.

@mehranzand
Copy link

When can I get package through the nuget?

@Mo-ha-mmed
Copy link

Hello guys.
Is there an alternative to ocelot for .net core 3.0

@TomPallister TomPallister merged commit 903b380 into ThreeMammals:develop Oct 28, 2019
@TomPallister
Copy link
Member

I will get a release out this morning before I go to work!

@saadshams
Copy link

updated nuget package please?

@jmezach
Copy link
Contributor

jmezach commented Nov 3, 2019

@saadshams New version is already available on NuGet (version 13.8.0) with compatibility for .NET Core 3.0. I have already successfully upgraded my project to 3.0 which uses Ocelot.

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.

Ocelot is not working with .NET Core 3