Limit context background #8093

cyberbono3 · 2020-12-05T22:55:03Z

Description

Found few cases for limiting context.Background()

Folllowing @robert-zaremba proposal:
-we can access context from function arguments or
-we can access context from parent function context

closes: #7834

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer
Review Codecov Report in the comment section below once CI passes

codecov · 2020-12-05T23:03:26Z

Codecov Report

Merging #8093 (2c47549) into master (c1b567f) will decrease coverage by 0.01%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master    #8093      +/-   ##
==========================================
- Coverage   61.50%   61.49%   -0.02%     
==========================================
  Files         651      651              
  Lines       37253    37266      +13     
==========================================
+ Hits        22912    22916       +4     
- Misses      11932    11945      +13     
+ Partials     2409     2405       -4

Impacted Files	Coverage Δ
client/keys/parse.go	`76.27% <0.00%> (ø)`
client/rpc/validators.go	`0.00% <0.00%> (ø)`
crypto/keyring/keyring.go	`61.21% <ø> (ø)`
client/keys/migrate.go	`43.47% <41.66%> (-10.37%)`	⬇️
x/slashing/client/cli/query.go	`43.24% <66.66%> (ø)`
x/staking/client/cli/query.go	`73.27% <92.30%> (ø)`
client/grpc/tmservice/block.go	`50.00% <100.00%> (ø)`
client/grpc/tmservice/service.go	`65.45% <100.00%> (ø)`
client/grpc/tmservice/status.go	`50.00% <100.00%> (ø)`
x/auth/client/cli/query.go	`34.90% <100.00%> (ø)`
... and 7 more

robert-zaremba

Good start. However this PR doesn't fully close the issue. There are more places where we need to update the code to reduce context.Background use in non test files (eg in /types, /x/...)

client/rpc/validators.go

cyberbono3 · 2020-12-08T23:31:19Z

Good start. However this PR doesn't fully close the issue. There are more places where we need to update the code to reduce context.Background use in non test files (eg in /types, /x/...)

Yeah, you are right. I have pushed the update.

robert-zaremba

utACK

github-actions · 2021-01-23T00:43:56Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

robert-zaremba · 2021-01-23T09:46:34Z

Let's resolve conflicts and finish the review. Let me know if you can finish this PR.

robert-zaremba · 2021-01-30T10:13:56Z

Let's reopen. @cyberbono3 -when are you planning to finish this?

robert-zaremba

There are still few not resolved comments.

cyberbono3 · 2021-02-01T21:52:18Z

@robert-zaremba all checks pass. Please take a look.

robert-zaremba

looks good. thanks.

robert-zaremba · 2021-02-01T23:05:01Z

client/grpc/tmservice/block.go

@@ -8,12 +8,12 @@ import (
 	"github.com/cosmos/cosmos-sdk/client"
 )

-func getBlock(clientCtx client.Context, height *int64) (*ctypes.ResultBlock, error) {
+func getBlock(ctx context.Context, clientCtx client.Context, height *int64) (*ctypes.ResultBlock, error) {


in fact we are working on making client.Context implementing context.Context. But it's on hold now.

Co-authored-by: Alessio Treglia <alessio@tendermint.com> Co-authored-by: sahith-narahari <sahithnarahari@gmail.com>

amaury1093

thanks, utACK. I'm a bit suspicous why client/docs/statik/statik.go was modified, @cyberbono3 can you explain?

If the user specifies an incorrect `--home`, then the old behaviour would automatically populate it with fresh values, but we should fail instead.

into limit-context

amaury1093 · 2021-02-03T09:46:47Z

client/keys/migrate.go

@@ -62,7 +62,7 @@ func runMigrateCmd(cmd *cobra.Command, args []string) error {

 	var (
 		tmpDir   string
-		migrator keyring.InfoImporter
+		migrator keyring.Importer


i'm not sure why github is showing diff of another PR...

I've seen it already in the past.

robert-zaremba · 2021-02-03T10:17:14Z

thanks, utACK. I'm a bit suspicous why client/docs/statik/statik.go was modified, @cyberbono3 can you explain?

I can respond on that. It inherits a context from command, rather than creating a new one.

amaury1093 · 2021-02-03T10:46:23Z

I can respond on that. It inherits a context from command, rather than creating a new one.

would prefer statik.go to be always generated, and never manually modified.

robert-zaremba · 2021-02-03T14:30:59Z

Ah, I didn't check the file is generated - good point. .... so we woould need to change the statik?

cyberbono3 · 2021-02-03T17:27:56Z

I have replaced modified statik.go with original statik.go from cosmos/master

cyberbono3 force-pushed the limit-context branch from 643df57 to 6ce5013 Compare December 5, 2020 23:53

robert-zaremba requested changes Dec 7, 2020

View reviewed changes

client/rpc/validators.go Outdated Show resolved Hide resolved

client/rpc/validators.go Outdated Show resolved Hide resolved

client/rpc/validators.go Outdated Show resolved Hide resolved

cyberbono3 force-pushed the limit-context branch from 81930bd to 8f439f1 Compare December 8, 2020 23:35

robert-zaremba approved these changes Dec 8, 2020

View reviewed changes

github-actions bot added the stale label Jan 23, 2021

robert-zaremba marked this pull request as ready for review January 23, 2021 09:45

robert-zaremba requested review from aaronc, alessio and alexanderbez as code owners January 23, 2021 09:45

robert-zaremba self-assigned this Jan 23, 2021

github-actions bot closed this Jan 30, 2021

robert-zaremba reopened this Jan 30, 2021

robert-zaremba requested changes Jan 30, 2021

View reviewed changes

cyberbono3 force-pushed the limit-context branch 3 times, most recently from 1876128 to aa4c41d Compare February 1, 2021 03:57

cyberbono3 added 4 commits February 1, 2021 10:09

limit context background

f1a3587

update

4b68904

fixed minor issues

a031535

fixed TestStatusCommand

03c9368

cyberbono3 force-pushed the limit-context branch from 9d187e3 to 03c9368 Compare February 1, 2021 18:10

robert-zaremba approved these changes Feb 1, 2021

View reviewed changes

robert-zaremba requested review from amaury1093 and blushi February 1, 2021 23:08

jgimeno and others added 2 commits February 2, 2021 09:59

Fix keyring import from older versions. (#8436)

f9da95a

Co-authored-by: Alessio Treglia <alessio@tendermint.com> Co-authored-by: sahith-narahari <sahithnarahari@gmail.com>

Add changelog (#8490)

15d83f5

amaury1093 approved these changes Feb 2, 2021

View reviewed changes

amaury1093 added Type: Code Hygiene General cleanup and restructuring of code to provide clarity, flexibility, and modularity. and removed stale labels Feb 2, 2021

michaelfig and others added 8 commits February 2, 2021 16:57

fix: tendermint subcommands should not create missing files (#8481)

ce161c4

If the user specifies an incorrect `--home`, then the old behaviour would automatically populate it with fresh values, but we should fail instead.

limit context background

d4e1d7b

update

8d6bad6

fixed minor issues

f3cd95c

fixed TestStatusCommand

070ee85

statik.go

53aebfd

Merge branch 'limit-context' of https://github.com/cyberbono3/cosmos-sdk

7562134

into limit-context

replaced static.go

2c47549

amaury1093 approved these changes Feb 3, 2021

View reviewed changes

amaury1093 added the A:automerge Automatically merge PR once all prerequisites pass. label Feb 3, 2021

mergify bot merged commit d37c590 into cosmos:master Feb 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Limit context background #8093

Limit context background #8093

cyberbono3 commented Dec 5, 2020

codecov bot commented Dec 5, 2020 •

edited

Loading

robert-zaremba left a comment

cyberbono3 commented Dec 8, 2020

robert-zaremba left a comment

github-actions bot commented Jan 23, 2021

robert-zaremba commented Jan 23, 2021

robert-zaremba commented Jan 30, 2021

robert-zaremba left a comment

cyberbono3 commented Feb 1, 2021

robert-zaremba left a comment

robert-zaremba Feb 1, 2021

amaury1093 left a comment

amaury1093 Feb 3, 2021

robert-zaremba Feb 3, 2021

robert-zaremba commented Feb 3, 2021

amaury1093 commented Feb 3, 2021

robert-zaremba commented Feb 3, 2021

cyberbono3 commented Feb 3, 2021

Limit context background #8093

Limit context background #8093

Conversation

cyberbono3 commented Dec 5, 2020

Description

codecov bot commented Dec 5, 2020 • edited Loading

Codecov Report

robert-zaremba left a comment

Choose a reason for hiding this comment

cyberbono3 commented Dec 8, 2020

robert-zaremba left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 23, 2021

robert-zaremba commented Jan 23, 2021

robert-zaremba commented Jan 30, 2021

robert-zaremba left a comment

Choose a reason for hiding this comment

cyberbono3 commented Feb 1, 2021

robert-zaremba left a comment

Choose a reason for hiding this comment

robert-zaremba Feb 1, 2021

Choose a reason for hiding this comment

amaury1093 left a comment

Choose a reason for hiding this comment

amaury1093 Feb 3, 2021

Choose a reason for hiding this comment

robert-zaremba Feb 3, 2021

Choose a reason for hiding this comment

robert-zaremba commented Feb 3, 2021

amaury1093 commented Feb 3, 2021

robert-zaremba commented Feb 3, 2021

cyberbono3 commented Feb 3, 2021

codecov bot commented Dec 5, 2020 •

edited

Loading