SnapshotDeleteRequest use []string instead of string #237

Jakob3xD · 2023-02-21T17:18:42Z

Description

SnapshotDeleteRequest use []string instead of string

Issues Resolved

Closes 236

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

VachaShah

Thank you for this fix @Jakob3xD! Can you also add a test for the same?

Jakob3xD · 2023-02-22T07:35:52Z

@VachaShah Are there any existing tests I can orientate on that handle snapshots or maybe something else, so I can see how you want the tests to look like and what structure they should have?

VachaShah · 2023-02-24T21:37:14Z

@VachaShah Are there any existing tests I can orientate on that handle snapshots or maybe something else, so I can see how you want the tests to look like and what structure they should have?

@Jakob3xD All the API tests currently exist in opensearchapi_integration_test.go.

dblock

This is a breaking change for the client. Can we avoid it and have both versions of string and string[] and implement the former using the latter everywhere one may want to pass a single item?

Jakob3xD · 2023-03-01T11:53:07Z

This is a breaking change but only because this was wrong in the first place. I should have opened my issue as bug and not as feature. IMO it is also easier to convert a string to []string by just doing []string{"Hello"} instead of converting a []string to string by doing strings.Join(stringvar, ",") which needs to import the strings lib.

dblock · 2023-03-01T16:01:15Z

@Jakob3xD You're right, still before I merge, is there a way to make the change backwards compatible? Does it make any sense to have two fields Snapshot and Shapshots for example and two functions?

Rebase please, I'm ok merging this otherwise.

Do we have the same problem elsewhere?

Jakob3xD · 2023-03-02T16:26:51Z

@dblock IMO it does not make sense to have both fields as it add unneeded overhead.
Other snapshot requests are snapshot specific and are therefore only a string.

I did't find more problems with wrong types.

@VachaShah to add integration tests for snapshots, I need to create a snapshot repository which would require setting path.repo in Opensearch and I am not sure how this is done in the github workflows. If you can help me enabling this in the CI I would add integration tests for:

snapshotRepo create/delete
Snapshot create,delete,restore,clone

dblock · 2023-03-02T22:32:25Z

Thanks @Jakob3xD for hanging in here with me. FYI, I am not trying to be difficult, I just want to make sure we are doing the right thing.

There's one important reason: you're introducing a breaking change. Every breaking change per semver requires a major version increment, that's a promise we have made - someone may be using the wrong type today and we will break them with this change. Or am I incorrect and the API doesn't work at all today? I would rather not major-increment the version for this.
I would be ok with any test that exercises this code. Something that mocks the server response is acceptable. But integration tests would be better. If you can't figure out how to do it I can try looking into it or maybe @VachaShah knows too.

Jakob3xD · 2023-03-03T17:44:53Z

I understand your point. It wouldn´t make sens to do a mayor release for this. Maybe we keep this PR back until the next mayor release?

As sad I am happy to write the tests. The only thing I need help with is setting the path.repo in the Opensearch config so I can create the snapshot repository.

dblock · 2023-03-06T21:37:17Z

I understand your point. It wouldn´t make sens to do a mayor release for this. Maybe we keep this PR back until the next mayor release?

Sure, but what's the reason not to add a backwards compatible version, and marking the wrong interface deprecated?

As sad I am happy to write the tests. The only thing I need help with is setting the path.repo in the Opensearch config so I can create the snapshot repository.

I am not sure how this works, give something a try and we'll help you when you get blocked?

wbeckler · 2023-03-15T06:02:30Z

Today, people might be sending in a comma-joined string like "snapshot1,snapshot2,snapshot3" as the parameter, since the function is just comma-joining the array.

Maybe we create a v3 branch of the client today and add this there, while marking the current type declaration as deprecated in the v2 client.

I kind of agree with @Jakob3xD that adding an alternative function with the new type is a bit of overhead. We would carry around SnapshotsDeleteRequest until it was deprecated, forcing a change on devs who would be scratching their heads about the repeat declaration. I think it's a better dev experience to let people use the comma-join workaround and add arrays to v3.

VachaShah · 2023-03-15T16:35:59Z

Hi @Jakob3xD sorry for the delay! As @dblock mentioned, atleast unit testing would be good to add where it mocks the server. For integration tests, how about adding the path.repo to the docker file which spins up OpenSearch cluster in the CI?

Signed-off-by: Jakob Hahn <jakob.hahn@hetzner.com>

Jakob3xD · 2023-03-17T17:16:14Z

@VachaShah I added some integration tests for Snapshot functions and added the setting to the docker file.
Works when testing locally.

dblock · 2023-03-17T17:44:34Z

Assuming the new integration tests work, I think we should make this as a breaking change and take #246 along the way and major increment the client version. I don't think we need to maintain and continue releasing the old line either.

dblock · 2023-03-17T17:46:53Z

I think we should introduce an UPGRADING.md in this project similar to https://github.com/opensearch-project/opensearch-py/blob/main/UPGRADING.md that will spell breaking changes. Want to add one as part of this PR @Jakob3xD?

Jakob3xD · 2023-03-17T18:24:56Z

The integration test with unrelased Opensearch fails, as the path setting is missing. If i see it correctly it builds opensearch from source and starts without any given settings. The normal Integration tests with docker works fine. Should I try adding the setting to the workflow https://github.com/opensearch-project/opensearch-go/blob/main/.github/workflows/test-integration-unreleased.yml#L52 ?

@dblock Yes, I will add the file.

dblock · 2023-03-17T18:35:48Z

The integration test with unrelased Opensearch fails, as the path setting is missing. If i see it correctly it builds opensearch from source and starts without any given settings. The normal Integration tests with docker works fine. Should I try adding the setting to the workflow https://github.com/opensearch-project/opensearch-go/blob/main/.github/workflows/test-integration-unreleased.yml#L52 ?

@dblock Yes, I will add the file.

Yes, do whatever you need to do to get a green build!

Signed-off-by: Jakob Hahn <jakob.hahn@hetzner.com>

dblock

I think we need to also add 3.0 to https://github.com/opensearch-project/opensearch-go/blob/main/COMPATIBILITY.md.

UPGRADING.md

dblock

I am good with this change!

@wbeckler This is the first time we're diverging a client version per semver with the server. Any concerns?

Signed-off-by: Jakob Hahn <jakob.hahn@hetzner.com>

dblock · 2023-03-22T18:47:10Z

@Jakob3xD looks like integration tests are failing :( care to take a look please?

Signed-off-by: Jakob Hahn <jakob.hahn@hetzner.com>

opensearch-project#237 Signed-off-by: Vacha Shah <vachshah@amazon.com>

#322) * Revert "Draft: Add Err() function to Response for detailed errors (#246)" This reverts commit 8d3cf4d. Signed-off-by: Vacha Shah <vachshah@amazon.com> * Fixing PIT APIs introduced in PR #253 since they use response.Err() Signed-off-by: Vacha Shah <vachshah@amazon.com> * Removing opensearchapi.Error usage from snapshot tests introduced in PR #237 Signed-off-by: Vacha Shah <vachshah@amazon.com> * Resolve Changelog conflicts Signed-off-by: Vacha Shah <vachshah@amazon.com> --------- Signed-off-by: Vacha Shah <vachshah@amazon.com>

opensearch-project#322) * Revert "Draft: Add Err() function to Response for detailed errors (opensearch-project#246)" This reverts commit 8d3cf4d. Signed-off-by: Vacha Shah <vachshah@amazon.com> * Fixing PIT APIs introduced in PR opensearch-project#253 since they use response.Err() Signed-off-by: Vacha Shah <vachshah@amazon.com> * Removing opensearchapi.Error usage from snapshot tests introduced in PR opensearch-project#237 Signed-off-by: Vacha Shah <vachshah@amazon.com> * Resolve Changelog conflicts Signed-off-by: Vacha Shah <vachshah@amazon.com> --------- Signed-off-by: Vacha Shah <vachshah@amazon.com>

#322) (#384) * Revert "Draft: Add Err() function to Response for detailed errors (#246)" This reverts commit 8d3cf4d. * Fixing PIT APIs introduced in PR #253 since they use response.Err() * Removing opensearchapi.Error usage from snapshot tests introduced in PR #237 * Resolve Changelog conflicts --------- Signed-off-by: Vacha Shah <vachshah@amazon.com>

Jakob3xD requested review from dblock, VijayanB, svencowart and VachaShah as code owners February 21, 2023 17:18

Jakob3xD force-pushed the snapshotDeleteRequest-slice branch 2 times, most recently from aba54d3 to 51631fc Compare February 21, 2023 17:20

VachaShah reviewed Feb 22, 2023

View reviewed changes

dblock requested changes Feb 28, 2023

View reviewed changes

Jakob3xD force-pushed the snapshotDeleteRequest-slice branch from 51631fc to d08fffd Compare March 2, 2023 15:32

Jakob3xD added 2 commits March 17, 2023 14:58

SnapshotDeleteRequest use []string instead of string

187192e

Signed-off-by: Jakob Hahn <jakob.hahn@hetzner.com>

Add opensearchapi snapshot ingegration test

1921267

Signed-off-by: Jakob Hahn <jakob.hahn@hetzner.com>

Jakob3xD force-pushed the snapshotDeleteRequest-slice branch from 5cc4697 to 1921267 Compare March 17, 2023 13:58

Add path.repo value to test-integration-unreleased workflow

8112185

Signed-off-by: Jakob Hahn <jakob.hahn@hetzner.com>

dblock reviewed Mar 20, 2023

View reviewed changes

UPGRADING.md Outdated Show resolved Hide resolved

dblock previously approved these changes Mar 20, 2023

View reviewed changes

Add UPGRADING.md

0a333d7

Signed-off-by: Jakob Hahn <jakob.hahn@hetzner.com>

Jakob3xD dismissed dblock’s stale review via 0a333d7 March 21, 2023 10:34

Jakob3xD force-pushed the snapshotDeleteRequest-slice branch from ce93e58 to 0a333d7 Compare March 21, 2023 10:34

dblock previously approved these changes Mar 21, 2023

View reviewed changes

Merge branch 'main' into snapshotDeleteRequest-slice

da6c6e2

dblock dismissed their stale review via da6c6e2 March 22, 2023 18:36

dblock previously approved these changes Mar 22, 2023

View reviewed changes

Jakob3xD dismissed dblock’s stale review via 9b51d13 March 23, 2023 09:00

opensearchapi: Fix integration test with new error handling

8825feb

Signed-off-by: Jakob Hahn <jakob.hahn@hetzner.com>

Jakob3xD force-pushed the snapshotDeleteRequest-slice branch from 9b51d13 to 8825feb Compare March 23, 2023 09:01

dblock approved these changes Mar 23, 2023

View reviewed changes

dblock merged commit 2d469d4 into opensearch-project:main Mar 23, 2023

Jakob3xD deleted the snapshotDeleteRequest-slice branch March 24, 2023 15:46

dblock mentioned this pull request Apr 3, 2023

Added @Jakob3xD as a co-maintainer. #270

Merged

VachaShah added a commit to VachaShah/opensearch-go that referenced this pull request May 18, 2023

Removing opensearchapi.Error usage from snapshot tests introduced in PR

c6a753a

opensearch-project#237 Signed-off-by: Vacha Shah <vachshah@amazon.com>

VachaShah added a commit to VachaShah/opensearch-go that referenced this pull request May 18, 2023

Removing opensearchapi.Error usage from snapshot tests introduced in PR

e3079ee

opensearch-project#237 Signed-off-by: Vacha Shah <vachshah@amazon.com>

This was referenced Oct 3, 2023

Revert "Draft: Add Err() function to Response for detailed errors (#2… #383

Closed

Revert "Draft: Add Err() function to Response for detailed errors (#2… #384

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SnapshotDeleteRequest use []string instead of string #237

SnapshotDeleteRequest use []string instead of string #237

Jakob3xD commented Feb 21, 2023 •

edited by dblock

Loading

VachaShah left a comment

Jakob3xD commented Feb 22, 2023

VachaShah commented Feb 24, 2023

dblock left a comment

Jakob3xD commented Mar 1, 2023

dblock commented Mar 1, 2023 •

edited

Loading

Jakob3xD commented Mar 2, 2023

dblock commented Mar 2, 2023 •

edited

Loading

Jakob3xD commented Mar 3, 2023

dblock commented Mar 6, 2023

wbeckler commented Mar 15, 2023

VachaShah commented Mar 15, 2023

Jakob3xD commented Mar 17, 2023

dblock commented Mar 17, 2023 •

edited

Loading

dblock commented Mar 17, 2023

Jakob3xD commented Mar 17, 2023

dblock commented Mar 17, 2023

dblock left a comment

dblock left a comment

dblock commented Mar 22, 2023

SnapshotDeleteRequest use []string instead of string #237

SnapshotDeleteRequest use []string instead of string #237

Conversation

Jakob3xD commented Feb 21, 2023 • edited by dblock Loading

Description

Issues Resolved

VachaShah left a comment

Choose a reason for hiding this comment

Jakob3xD commented Feb 22, 2023

VachaShah commented Feb 24, 2023

dblock left a comment

Choose a reason for hiding this comment

Jakob3xD commented Mar 1, 2023

dblock commented Mar 1, 2023 • edited Loading

Jakob3xD commented Mar 2, 2023

dblock commented Mar 2, 2023 • edited Loading

Jakob3xD commented Mar 3, 2023

dblock commented Mar 6, 2023

wbeckler commented Mar 15, 2023

VachaShah commented Mar 15, 2023

Jakob3xD commented Mar 17, 2023

dblock commented Mar 17, 2023 • edited Loading

dblock commented Mar 17, 2023

Jakob3xD commented Mar 17, 2023

dblock commented Mar 17, 2023

dblock left a comment

Choose a reason for hiding this comment

dblock left a comment

Choose a reason for hiding this comment

dblock commented Mar 22, 2023

Jakob3xD commented Feb 21, 2023 •

edited by dblock

Loading

dblock commented Mar 1, 2023 •

edited

Loading

dblock commented Mar 2, 2023 •

edited

Loading

dblock commented Mar 17, 2023 •

edited

Loading