feat: add low-level dependency injection container API #9658

aaronc · 2021-07-09T00:55:39Z

Description

This PR defines the low-level API for the dependency injector container to be used for the new app.go wiring discussed in #9182, demoed in #9529, and refined by the App Wiring Working Group.

To see example usage, see container_test.go. All tests are expected to fail as there is no implementation here, just API. Thus the tests are skipped.

This API is inspired by https://github.com/uber-go/dig and https://github.com/uber-go/fx and is intended to be for low-level components, with a higher-level Module infrastructure sitting on top of this.

The container package here is defined as a standalone go module so that this code-base is isolated and self-contained and concerned with doing one thing (dependency injection) well.

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

…tainer-init

aaronc · 2021-07-22T19:30:08Z

Note that we will need to add github actions to cover all nested go modules (as in regen-network/regen-ledger#357). @cyberbono3 would you be able to support with that?

container/container_test.go

aaronc · 2021-07-23T15:13:37Z

container/struct_args.go

+// directly, but will instead use the struct's fields to resolve or populate
+// dependencies. Types with embedded StructArgs can be used in both the input
+// and output parameter positions.
+type StructArgs struct{}


I thought it was simpler to have one struct vs dig.In and dig.Out... Wonder if anyone has an opinion

In / Out makes it more clear when reading a structure definition, without going into the provider / usage:

when I see a struct withdig.In I quickly associate it with a dependencies of some type.

when I see a struct with dig.Out I see that those are some dependencies a provider can provide.

I'm happy to change it to In/Out, but note that I also have structs named Input and Output here to describe the actual constructor inputs/outputs... Any suggestions on naming?

…nc/container-init

codecov · 2021-07-26T18:55:22Z

Codecov Report

Merging #9658 (d553263) into master (0bcce5d) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #9658   +/-   ##
=======================================
  Coverage   63.60%   63.60%           
=======================================
  Files         568      568           
  Lines       53509    53511    +2     
=======================================
+ Hits        34034    34037    +3     
+ Misses      17550    17549    -1     
  Partials     1925     1925

Impacted Files	Coverage Δ
store/types/gas.go	`84.44% <ø> (ø)`
store/types/store.go	`68.75% <ø> (ø)`
x/upgrade/types/plan.go	`96.29% <ø> (ø)`
x/upgrade/abci.go	`94.73% <100.00%> (+0.14%)`	⬆️
x/upgrade/keeper/keeper.go	`80.18% <100.00%> (+0.08%)`	⬆️
crypto/keys/internal/ecdsa/privkey.go	`84.21% <0.00%> (+1.75%)`	⬆️

tac0turtle

Approval meant to move things forward as follow-up PRs have already been opened

blushi

Approving, maybe we can try to start some live code review of #9666 in the next WG call then?

robert-zaremba · 2021-08-11T22:02:31Z

container/struct_args.go

+// directly, but will instead use the struct's fields to resolve or populate
+// dependencies. Types with embedded StructArgs can be used in both the input
+// and output parameter positions.
+type StructArgs struct{}


In / Out makes it more clear when reading a structure definition, without going into the provider / usage:

when I see a struct withdig.In I quickly associate it with a dependencies of some type.

when I see a struct with dig.Out I see that those are some dependencies a provider can provide.

aaronc · 2021-08-11T22:26:40Z

How about let's merge. I can do a live walkthrough of the next PR tomorrow, and we can discuss any naming details we want to tweak.

## Description closes #9775 needs #9658 --- ### Author Checklist *All items are required. Please add a note to the item if the item is not applicable and please add links to any relevant follow up issues.* I have... - [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting)) - [ ] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] updated the relevant documentation or specification - [ ] reviewed "Files changed" and left comments if necessary - [ ] confirmed all CI checks have passed ### Reviewers Checklist *All items are required. Please add a note if the item is not applicable and please add your handle next to the items reviewed if you only reviewed selected items.* I have... - [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] confirmed `!` in the type prefix if API or client breaking change - [ ] confirmed all author checklist items have been addressed - [ ] reviewed state machine logic - [ ] reviewed API design and naming - [ ] reviewed documentation is accurate - [ ] reviewed tests and test coverage - [ ] manually tested (if applicable)

aaronc added 2 commits July 8, 2021 20:55

feat: add dependency injection container API

8c8d996

define handler group type

02dcfae

aaronc changed the title ~~feat: add dependency injection container API~~ feat: add low-level dependency injection container API Jul 9, 2021

aaronc added 3 commits July 8, 2021 21:00

add go.mod

d73f9df

WIP on API

32c8ce4

WIP

b873faf

aaronc mentioned this pull request Jul 9, 2021

feat: implement low-level dependency injection container #9666

Merged

19 tasks

aaronc and others added 4 commits July 9, 2021 12:37

WIP on API

a26539d

Merge branch 'master' of github.com:cosmos/cosmos-sdk into aaronc/con…

bf08a30

…tainer-init

update docs

9c65775

Merge branch 'master' into aaronc/container-init

d7773fa

aaronc marked this pull request as ready for review July 22, 2021 19:27

aaronc requested a review from alexanderbez as a code owner July 22, 2021 19:27

aaronc requested review from fdymylja, jgimeno, tac0turtle, robert-zaremba and blushi July 22, 2021 19:27

blushi reviewed Jul 23, 2021

View reviewed changes

container/container_test.go Show resolved Hide resolved

aaronc commented Jul 23, 2021

View reviewed changes

aaronc and others added 4 commits July 26, 2021 11:18

rename

211c756

Merge branch 'master' into aaronc/container-init

aebc9aa

Add unit tests for nested modules

9ec37b6

Merge remote-tracking branch 'origin/aaronc/container-init' into aaro…

392fb3f

…nc/container-init

aaronc added the C:depinject Issues and PR related to depinject label Jul 26, 2021

update labeler.yml

125a275

aaronc removed the C:depinject Issues and PR related to depinject label Jul 26, 2021

update labeler.yml

887aca5

github-actions bot added the Type: Build label Jul 26, 2021

github-actions bot added the T: CI label Jul 26, 2021

aaronc added the C:depinject Issues and PR related to depinject label Jul 26, 2021

Merge branch 'master' into aaronc/container-init

5f786f0

robert-zaremba requested a review from clevinson July 29, 2021 15:37

robert-zaremba mentioned this pull request Jul 29, 2021

Simplified App Wiring #9773

Closed

5 tasks

clevinson assigned ryanchristo Aug 5, 2021

tac0turtle approved these changes Aug 10, 2021

View reviewed changes

blushi approved these changes Aug 10, 2021

View reviewed changes

robert-zaremba approved these changes Aug 11, 2021

View reviewed changes

Merge branch 'master' into aaronc/container-init

d553263

aaronc added the A:automerge Automatically merge PR once all prerequisites pass. label Aug 11, 2021

aaronc merged commit e656d5e into master Aug 12, 2021

aaronc deleted the aaronc/container-init branch August 12, 2021 13:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add low-level dependency injection container API #9658

feat: add low-level dependency injection container API #9658

aaronc commented Jul 9, 2021 •

edited

Loading

aaronc commented Jul 22, 2021 •

edited

Loading

aaronc Jul 23, 2021

robert-zaremba Aug 11, 2021

aaronc Aug 11, 2021

codecov bot commented Jul 26, 2021 •

edited

Loading

tac0turtle left a comment •

edited

Loading

blushi left a comment

robert-zaremba Aug 11, 2021

aaronc commented Aug 11, 2021 •

edited

Loading

feat: add low-level dependency injection container API #9658

feat: add low-level dependency injection container API #9658

Conversation

aaronc commented Jul 9, 2021 • edited Loading

Description

Author Checklist

Reviewers Checklist

aaronc commented Jul 22, 2021 • edited Loading

aaronc Jul 23, 2021

Choose a reason for hiding this comment

robert-zaremba Aug 11, 2021

Choose a reason for hiding this comment

aaronc Aug 11, 2021

Choose a reason for hiding this comment

codecov bot commented Jul 26, 2021 • edited Loading

Codecov Report

tac0turtle left a comment • edited Loading

Choose a reason for hiding this comment

blushi left a comment

Choose a reason for hiding this comment

robert-zaremba Aug 11, 2021

Choose a reason for hiding this comment

aaronc commented Aug 11, 2021 • edited Loading

aaronc commented Jul 9, 2021 •

edited

Loading

aaronc commented Jul 22, 2021 •

edited

Loading

codecov bot commented Jul 26, 2021 •

edited

Loading

tac0turtle left a comment •

edited

Loading

aaronc commented Aug 11, 2021 •

edited

Loading