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

Avoid using custom version requirements #177

Merged

Conversation

rukai
Copy link
Contributor

@rukai rukai commented Sep 1, 2023

Using custom version requirements in a library is generally considered a bad idea as it can lead to broken builds downstream.
The cargo docs goes into this in the box starting with:

Recommendation: When in doubt, use the default version requirement operator.
...

In this PR I have removed all non-default version requirements.
Happy to discuss if you feel some are needed.


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.

Signed-off-by: Lucas Kent <rubickent@gmail.com>
@rukai rukai force-pushed the avoid_using_custom_version_specifiers branch from 1fce6c4 to 4ca2cde Compare September 1, 2023 03:00
@codecov
Copy link

codecov bot commented Sep 1, 2023

Codecov Report

Merging #177 (b6756ee) into main (d78b01d) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #177      +/-   ##
==========================================
- Coverage   73.84%   73.83%   -0.01%     
==========================================
  Files         402      401       -1     
  Lines       63630    63630              
==========================================
- Hits        46986    46983       -3     
- Misses      16644    16647       +3     
Flag Coverage Δ
integration 73.83% <ø> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 3 files with indirect coverage changes

Copy link
Collaborator

@Xtansia Xtansia left a comment

Choose a reason for hiding this comment

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

Hi @rukai,

Appreciate the PR! I've just gone through and mostly for my own sake commented on the various changes about whether they remain equivalent etc. There's only one problematic change which is those for the aws-* crates where we want to "float up" to the version used by the consumer as 0. versions behave differently with the default operator than 1. (or greater) do.

ie.
"1.53" == ">= 1.53.0, < 2.0.0"
"0.53" == ">= 0.53.0, < 0.54.0"

api_generator/Cargo.toml Show resolved Hide resolved
api_generator/Cargo.toml Show resolved Hide resolved
api_generator/Cargo.toml Show resolved Hide resolved
api_generator/Cargo.toml Show resolved Hide resolved
api_generator/Cargo.toml Show resolved Hide resolved
yaml_test_runner/Cargo.toml Show resolved Hide resolved
yaml_test_runner/Cargo.toml Show resolved Hide resolved
yaml_test_runner/Cargo.toml Show resolved Hide resolved
yaml_test_runner/Cargo.toml Show resolved Hide resolved
yaml_test_runner/Cargo.toml Show resolved Hide resolved
@rukai
Copy link
Contributor Author

rukai commented Sep 1, 2023

I've undone the aws changes since, as you've correctly identified, that would be a breaking change.

That said specifying the AWS versions like that is quite dangerous because they can freely issue breaking changes within any 0.x.0 releases.
I dont have a good solution for you though since you cant issue a breaking release until opensearch itself does given this policy: https://docs.rs/opensearch/latest/opensearch/#versions-and-compatibility
But its also not really my concern as I dont enable the aws-auth feature anyway.

Copy link
Collaborator

@Xtansia Xtansia left a comment

Choose a reason for hiding this comment

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

@rukai Thanks for reverting the aws-* versioning, looks good now, just missing --signoff on your most recent commit, if you could amend that then that'd be great.

I'm aware that a breaking change could be introduced in any 0.x.0 version of aws-* however it is somewhat of a calculated risk for the time being as the parts we use are mostly stable, and the end user has control of what aws-* version is brought in and can roll-back or pin as a work around if a breakage did come. But I have been looking for mechanism to better encapsulate this dependency and the API usage of it, which if you have ideas or opinions on good ways to achieve that I'd love to hear your thoughts.

Additionally that version mandate about being lockstep with the server major versioning, is a relic of the fork from Elasticsearch and is not necessarily true for today so I've created an issue to update that.

@Xtansia Xtansia added the skip-changelog Skip changelog verification label Sep 3, 2023
Signed-off-by: Lucas Kent <rubickent@gmail.com>
@rukai rukai force-pushed the avoid_using_custom_version_specifiers branch from e04f7dc to b6756ee Compare September 4, 2023 01:57
@rukai
Copy link
Contributor Author

rukai commented Sep 4, 2023

oops, sign off should be fixed now.

@Xtansia Xtansia merged commit 206dba9 into opensearch-project:main Sep 4, 2023
30 of 31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip-changelog Skip changelog verification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants