Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #501 +/- ##
=======================================
Coverage 35.81% 35.81%
=======================================
Files 25 25
Lines 2284 2284
=======================================
Hits 818 818
Misses 1394 1394
Partials 72 72 ☔ View full report in Codecov by Sentry. |
alecjacobs5401
left a comment
alecjacobs5401
left a comment
There was a problem hiding this comment.
Changes all seem reasonable here! Mainly have a a recurring open question about the nature of this PR introducing breaking changes to almost every public interface. I know this is slated for a v3 release, but should this PR be split up/the title include the nature of the breaking changes? I believe the CHANGELOG is auto generated based on commits now, so want to make sure we're building up a good changelog for migrations.
I know its more work too, but a well worn pattern I've seen is to add a <Action>WithContext as a way to side-step the breaking nature of passing contexts through. Is that something we might need to consider?
| // collisions will be populated with any keys that get overwritten | ||
| func (e *Environ) Load(s store.Store, service string, collisions *[]string) error { | ||
| return e.load(s, service, collisions, false) | ||
| func (e *Environ) Load(ctx context.Context, s store.Store, service string, collisions *[]string) error { |
There was a problem hiding this comment.
as it stands, anywhere we add in ctx here as a starting argument is considered a breaking change since it alters the public interface.
I know we're preparing the default branch as a candidate for v3, so maybe its moot, but does it make sense to call this out heavily somewhere (either in a changelog, or updating the merge commit) to designate the nature of these breaking changes?
There was a problem hiding this comment.
I'm fine updating the commit to be a breaking change in conventional commit-ese. It is certainly a significant break for anyone using chamber as a library, which I wasn't thinking about.
There was a problem hiding this comment.
Commit updated to call out that this is a breaking change.
There was a problem hiding this comment.
As for <Action>WithContext: I'm OK with using the breaking change opportunity to avoid adding those extra functions to preserve compatibility. It'd certainly be a lot of extra functions.
bhavanki
changed the title
The new AWS SDK uses contexts for all API functions and for loading the SDK configuration. When chamber was migrated to the SDK, all of them were set to `context.TODO()`. This commit removes those temporary values and threads contexts properly throughout the codebase, starting from the chamber commands. BREAKING CHANGE: This commit changes the signature of many exported functions to add a context argument.
3 participants
The new AWS SDK uses contexts for all API functions and for loading the
SDK configuration. When chamber was migrated to the SDK, all of them
were set to
context.TODO(). This commit removes those temporary valuesand threads contexts properly throughout the codebase, starting from the
chamber commands.
BREAKING CHANGE: This commit changes the signature of many exported
functions to add a context argument.