Skip to content
This repository has been archived by the owner on Jun 1, 2024. It is now read-only.

Elasticsearch.Net 7.0.0 support, V7 templates #249

Merged
merged 2 commits into from
Jul 15, 2019

Conversation

kanadaj
Copy link
Contributor

@kanadaj kanadaj commented May 29, 2019

What issue does this PR address?
#248 targeting Elasticsearch.Net 7.0.0-alpha2 on .NET Standard 2.0; still targeting 6.0.0 on .NET Standard 1.1 and .NET Framework 4.5.1 since 7.0.0 is not compatible.

A small note: why target .NET Framework separately?

Does this PR introduce a breaking change?
Possibly. I cannot test against ES 6 right now so would need verification whether the 7.0.0 SDK of ElasticSearch breaks it or not.

Other information:
Needs testing against ES 6.X.

@kanadaj kanadaj mentioned this pull request May 29, 2019
@@ -1,5 +1,5 @@
{
"sdk": {
"version": "2.1.4"
"version": "2.1.500"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why 500 here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ffMathy oh that's just from my initial setup cause I don't have 2.1.4 locally and when I did that I was just experimenting, didn't consider I'd end up making a PR...

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see.

@dominiclapointe
Copy link

Is there a reason that it has not been merged yet? Really looking forward for the next nuget package that is compatible with esv7!!

@kanadaj
Copy link
Contributor Author

kanadaj commented Jun 4, 2019

@dominiclapointe Yes, the fact that this pull request also contains a dependency on the Preview version of the SDK itself, and causes some other complications like leaving the dependency of the netcore1.3 package as 6.X and netcore2.0 as 7.X - they kind of have to figure out what to do with netcore1.3.

Also, I'm still not sure the preview SDK for 7.X is compatible at all with ES6

@naymore
Copy link

naymore commented Jun 18, 2019

Does anybody know what Elastic's idea is with its dotnet SDK?
Is it backward compatible?
Will 6.8 receive future updates (hotfix releases like 6.8.1, ..)? (In the past this almost never happened)
Does the ES SDK for dotnet version match the supported ES version?

ES v6 will be supported for quite some time, see EOL, however v7 introduced breaking changes and v8 will introduce even more.

btw, 7.0.0-beta1 has been released ;-)
Maybe this sink should also reflect the ES SDK used in its version scheme.
v7.1.0 sink only supports v6.8 ES SDK :|

It might be simpler to have ES-version-specific sinks (at least for ES v6.x and v7+) rather than having one sink for any ES version and also be in doubt what's even supported or breaking :)

@mivano
Copy link
Contributor

mivano commented Jun 18, 2019

@Mpdreamz can you give some feedback here about the plans?

@NeilCross
Copy link

Elasticsearch.net 7.0.0 has been released on nuget recently: https://www.nuget.org/packages/Elasticsearch.Net/

@Mpdreamz
Copy link
Contributor

Mpdreamz commented Jul 4, 2019

@mivano will open up a new ticket about some of the plans we discussed 😸

I've started on a continuation of this PR here:

https://github.com/Mpdreamz/serilog-sinks-elasticsearch/tree/feature/7.x-support

This also includes a new dedicate integration test project which uses the same testing framework of the client to spin up different Elasticsearch versions.

See here for an example integration test:

https://github.com/Mpdreamz/serilog-sinks-elasticsearch/blob/feature/7.x-support/Serilog.Sinks.Elasticsearch.IntegrationTests/Elasticsearch7X.cs

For now there are only 7.x tests but will add 6.x tests as well.

@mivano mivano merged commit f62bc73 into serilog-contrib:dev Jul 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants