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

subcmd/apply: Support fail-safe approach #185

Conversation

shimon-armis
Copy link

Description

Up until now, the apply command implemented a fail-fast strategy. This means that an attempt to configure a batch of topics (by passing a path to a folder contains a list of topics configurations files) will be terminated upon the first failure (w/o even trying to apply the configuration for the next-in-line topics).
This commit introduces the fail-safe support:
It allows the user to run the apply command in a fail-safe manner, which means that instead of failing upon the first failure, we'll simply continue to the next topic configuration, while aggregating all the errors along the way.
Eventually, if there were any errors, all of them will be shown.

@shimon-armis shimon-armis requested a review from a team as a code owner March 21, 2024 22:26
@shimon-armis
Copy link
Author

@hhahn-tw Can you please review this one? :)

@shimon-armis
Copy link
Author

@hhahn-tw @petedannemann Can you guys maybe take a look?

@shimon-armis
Copy link
Author

@jkoelker Can you maybe review this PR?

Copy link
Member

@jkoelker jkoelker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just a reccomendation to use errors.Join to concat the errors, if not, you can also use fmt.Errorf with multiple %w to achieve the same thing and not drop the error context by rendering it as a string.

cmd/topicctl/subcmd/apply.go Outdated Show resolved Hide resolved
cmd/topicctl/subcmd/apply.go Outdated Show resolved Hide resolved
cmd/topicctl/subcmd/apply.go Outdated Show resolved Hide resolved
@shimon-armis shimon-armis force-pushed the shimon.support_fail_safe_approach_for_the_apply_cmd branch from 44deeff to 1bf5d02 Compare April 2, 2024 06:17
@shimon-armis
Copy link
Author

shimon-armis commented Apr 2, 2024

LGTM, just a reccomendation to use errors.Join to concat the errors, if not, you can also use fmt.Errorf with multiple %w to achieve the same thing and not drop the error context by rendering it as a string.

errors.Join() is only supported from go 1.20 (this repo is running v1.18).
Also, fmt.Errorf() doesn't support multiple %w (as explained here. Here's the error I got in the PR check:
Error: cmd/topicctl/subcmd/apply.go:182:12: fmt.Errorf call has more than one error-wrapping directive %w
make: *** [Makefile:20: vet] Error 1

@shimon-armis shimon-armis force-pushed the shimon.support_fail_safe_approach_for_the_apply_cmd branch from 1bf5d02 to 430b161 Compare April 2, 2024 08:10
Up until now, the `apply` command implemented a fail-fast
strategy. This means that an attempt to configure a batch
of topics (by passing a path to a folder contains a list
of topics configurations files) will be terminated upon
the first failure (w/o even trying to apply the configuration
for the next-in-line topics).
This commit introduces the fail-safe support:
It allows the user to run the `apply` command in a fail-safe
manner, which means that instead of failing upon the first
failure, we'll simply continue to the next topic configuration,
while aggregating all the errors along the way.
Eventually, if there were any errors, all of them will be shown.

Signed-off-by: shimon-armis <shimon.turjeman@armis.com>
@shimon-armis shimon-armis force-pushed the shimon.support_fail_safe_approach_for_the_apply_cmd branch from 430b161 to 661868a Compare April 2, 2024 09:28
@shimon-armis
Copy link
Author

@hhahn-tw Can you please merge this one?

@shimon-armis
Copy link
Author

@hhahn-tw ?

@hhahn-tw
Copy link
Contributor

hhahn-tw commented Apr 5, 2024

Thanks once more for your contribution!

@hhahn-tw hhahn-tw merged commit 4b41f0b into segmentio:master Apr 5, 2024
9 checks passed
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.

4 participants