feat: support not enabling aws default features #46
Conversation
arielb1
force-pushed
the
no-aws-defaults
branch
15 times, most recently
from
4835f18 to
7d126f2
Compare
jlizen
left a comment
jlizen
left a comment
There was a problem hiding this comment.
Core implementation looks good but need:
- confirmation of testing of existing functionality without new features
- cleanup of the example code w/r/t conditional compilation noise
| - "1.85" # Current MSRV due to Cargo MSRV feature | ||
| - stable | ||
| flags: | ||
| - "--all-features" |
There was a problem hiding this comment.
I was surprised to see that these passed out of box, but I guess we don't actually test any network calls? Looks like we validate metadata parsing only, and nothing to do with s3?
Probably having an integration test that validates these directly e2e using real auth would be nice, but separate scope. I can cut an issue (low priority).
Without that, can you confirm that you've manually validated that the existing functionality (without the new features) works properly? IE no regression for old APIs.
There was a problem hiding this comment.
yea, the tests don't have network access. I'm not in the mood of setting up an AWS account so I can do AWS-in-the-loop tests.
There was a problem hiding this comment.
No worries, we can maybe do #47 someday
From offline - thanks for confirming that you tested locally e2e and existin gfunctionality works.
CHANGELOG.md
Outdated
|
|
||
| - add support for stopping the profiler | ||
| - make native memory profiling interval configurable | ||
| - add Cargo features to allow not enabling AWS/reqwest default features |
There was a problem hiding this comment.
this should go in unreleased section no? also can we add the new feature name(s) into this verbiage?
There was a problem hiding this comment.
I thought the 0.1.5 release failed. Apparently it fake failed. Will remove it from the changelog, and release-plz will re-insert it.
| .args(["local", "bucket"]), | ||
| ))] | ||
| struct Args { | ||
| #[cfg(feature = "s3-no-defaults")] |
There was a problem hiding this comment.
All of these feature flags make the example very hard to follow. Can we move all s3 features into their own struct, #[derive(Arg)] on it, and flatten it from the parent struct? That way we can just feature gate the one field in the top-level struct.
examples/simple/main.rs
Outdated
| args.bucket_owner, | ||
| args.profiling_group, | ||
| ) { | ||
| #[cfg(feature = "s3-no-defaults")] |
There was a problem hiding this comment.
This is pretty unreadable as well. Can we refactor to only need one of each cfg flag (enabled vs not enabled)?
For instance, add a struct that we derive default on and use default implementation for if feature is disabled. Or delegate to a function.
2 participants
📬 Issue #, if available:
✍️ Description of changes:
🔏 By submitting this pull request