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

[Bug] Add rt-tokio feature to aws-sdk-s3 #1248

Conversation

adam-singer
Copy link
Contributor

@adam-singer adam-singer commented Aug 8, 2024

Description

Feature rt-tokio had been transitively dependended on, durning refactors of Cargo.toml files it was accidentally dropped.

Fixes #1247

Type of change

Please delete options that aren't relevant.

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Will be deployed to staging systems.


This change is Reviewable

Feature `rt-tokio` had been transitively dependended on,
durning refactors of `Cargo.toml` files it was accidentally
dropped.
@adam-singer adam-singer requested a review from aaronmondal August 8, 2024 07:07
@MarcusSorealheis
Copy link
Collaborator

Thank you for fixing this one. It goes without saying that this will be in our point release.

Copy link
Member

@aaronmondal aaronmondal left a comment

Choose a reason for hiding this comment

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

It would be nice to have some sort of regression test for this (or at least an issue that tracks implementation of a regression test).

I'll create a point release after this is merged.

:lgtm:

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: 1 of 1 LGTMs obtained, and all files reviewed, and 1 discussions need to be resolved


nativelink-store/Cargo.toml line 17 at r1 (raw file):

  "rustls",
] }
aws-sdk-s3 = { version = "1.41.0", features = [

nit: Is there a way we could add a regression test or something? Maybe just a feature-check in the source for the S3 store so that builds fail early if this accidentally removed again in the future?

Copy link
Member

@allada allada left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 2 of 1 LGTMs obtained, and all files reviewed, and 1 discussions need to be resolved


nativelink-store/Cargo.toml line 17 at r1 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

nit: Is there a way we could add a regression test or something? Maybe just a feature-check in the source for the S3 store so that builds fail early if this accidentally removed again in the future?

This should have been caught in CI, the reason it didnt was because aws-sdk-s3 has to be defined in dev-dependencies (below) as well as here, but only got enabled below.

Writing a regression test for this is not going to be easy, because it will require a full integration test.

Copy link
Contributor Author

@adam-singer adam-singer left a comment

Choose a reason for hiding this comment

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

The test could of been something that asserts what the defaults provided from the library, as @allada it was a feature enabled in dev deps so we would still have missed it. We could.. put an assert in the main code upon init to halt if the credential provider util isn't returning expected sleep functions, don't think its the right place, integration tests would be better.

Reviewable status: 2 of 1 LGTMs obtained, and all files reviewed, and 1 discussions need to be resolved

@adam-singer adam-singer merged commit 3eadab0 into TraceMachina:main Aug 8, 2024
27 checks passed
@adam-singer adam-singer deleted the adams/enable-rt-tokio-nativelink-store branch August 8, 2024 16:15
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.

[Bug] Please provide a sleep_impl on the config, or disable timeouts.
4 participants