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

Resolve context.TODO() and add context argument #154

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

abicky
Copy link
Contributor

@abicky abicky commented Feb 27, 2025

Description

This PR is the third phase and part of the fourth phase of the integration with OpenTelemetry described on #153.

In the first part of the commits, I have added a context argument to the methods called from various code and added context.TODO() to each caller. I have also made the downstream methods (the methods called from the method) use the context argument appropriately and added a context argument if necessary.

In the second part of the commits, I have replaced context.TODO() with cmd.Context() in the cmd package and made the downstream methods use the context argument appropriately.

In the following PRs (the fourth phase), I will do the same thing in other repositories.

Behavior changes

This PR also makes the relayer shutdown gracefully (526ab84) when it receives SIGINT and SIGTERM.
Here is the output from the relayer started by service start when I pressed ^C:

Before

^C

After

^C{"time":"2025-02-27T18:13:43.798834915+09:00","level":"INFO","source":{"function":"github.com/hyperledger-labs/yui-relayer/cmd.notifyContext.func1","file":"/home/arabiki/.local/share/mise/installs/go/1.23.2/packages/pkg/mod/github.com/abicky/[email protected]/cmd/root.go","line":141},"msg":"Received SIGINT. Shutting down..."}
2025/02/27 18:13:44 context canceled

What I have checked

I have confirmed that there is no longer context.TODO() as follows:

% git grep 'context\.TODO' | wc
      0       0       0

I have also ensured that the CI passes in the following repositories:

@abicky abicky force-pushed the use-context-appropriately branch 2 times, most recently from b1c0ca7 to f526dce Compare February 27, 2025 07:33
@abicky abicky force-pushed the use-context-appropriately branch from f526dce to e71561e Compare February 27, 2025 07:54
@abicky abicky force-pushed the use-context-appropriately branch from e71561e to 6cdff2b Compare February 27, 2025 08:02
@abicky abicky force-pushed the use-context-appropriately branch 2 times, most recently from 25566ea to 609e560 Compare February 27, 2025 08:30
@abicky abicky force-pushed the use-context-appropriately branch from 609e560 to 526ab84 Compare February 27, 2025 08:37
Signed-off-by: Takeshi Arabiki <[email protected]>
To avoid the error "Cannot open: File exists"
cf. https://github.com/orgs/community/discussions/58174

actions/setup-go v4 or later has its own caching mechanism,
so we don't need actions/cache.

Signed-off-by: Takeshi Arabiki <[email protected]>
@abicky abicky force-pushed the use-context-appropriately branch from 34f76fc to 0b719e8 Compare February 27, 2025 09:02
@abicky abicky marked this pull request as ready for review February 27, 2025 09:02
@abicky abicky requested a review from a team as a code owner February 27, 2025 09:02
@abicky
Copy link
Contributor Author

abicky commented Feb 27, 2025

@siburu @dai1975 Can you review the changes?

@abicky abicky force-pushed the use-context-appropriately branch from c4bc26f to 1df3281 Compare February 28, 2025 00:02
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.

1 participant