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

refactor: break main into functions #123

Merged
merged 2 commits into from
Sep 19, 2024
Merged

Conversation

abuchanan-airbyte
Copy link
Collaborator

Just chipping away at some minor code cleanup.

@abuchanan-airbyte abuchanan-airbyte requested a review from a team as a code owner September 18, 2024 19:45
@@ -20,7 +22,13 @@ type doer interface {
// This is accomplished by fetching the latest github tag and comparing it to the version provided.
// Returns the latest version, or an empty string if we're already running the latest version.
// Will return ErrDevVersion if the build.Version is currently set to "dev".
func Check(ctx context.Context, doer doer, version string) (string, error) {
func Check() (string, error) {
Copy link
Member

Choose a reason for hiding this comment

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

This should still accept an external context.Context. The reason being we want to be able to cancel all the contexts by canceling the one root context.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

By the time you cancel the parent context in the CLI, the code has called os.Exit already.

But, I could still pipe it through if you think it's important.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's still worth doing.

@abuchanan-airbyte abuchanan-airbyte merged commit f5b5265 into main Sep 19, 2024
2 checks passed
@abuchanan-airbyte abuchanan-airbyte deleted the abuch/update-refactor branch September 19, 2024 16:05
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.

2 participants