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

[local-app] Cleanup run command, apply recommended go style #8956

Merged
merged 1 commit into from
Apr 6, 2022

Conversation

easyCZ
Copy link
Member

@easyCZ easyCZ commented Mar 25, 2022

Description

  • Restructure run arguments into an object, easier extension and visual grepping
  • Use CamelCase for constants
  • Use the same logic of temp definition across platforms, os.Temp already produces /tmp on darwin and linux.

How to test

cd components/local-app
go run main.go

Release Notes

NONE

@akosyakov
Copy link
Member

@mustard-mh Could you finish a review please?

@mustard-mh
Copy link
Contributor

@mustard-mh Could you finish a review please?

Need more input from @easyCZ about #8956 (comment)

cc @akosyakov

@easyCZ
Copy link
Member Author

easyCZ commented Mar 30, 2022

@mustard-mh Thanks for the review. I've incorporated your changes and split out the port acquire into a seperate method to make it a bit cleaner.

}

defer func() {
if closeErr := rl.Close(); closeErr != nil {
logrus.WithField("port", port).WithError(closeErr).Warn("Failed to to close listener")
Copy link
Contributor

@mustard-mh mustard-mh Mar 30, 2022

Choose a reason for hiding this comment

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

WARN[0005] Failed to to close listener error="close tcp4 127.0.0.1:63110: use of closed network connection" port=63110

After test, we can see this warning every time. we don't need this warning (check err is closed first), because it will always appear if auth works(it will close by this one too).

Copy link
Contributor

@mustard-mh mustard-mh left a comment

Choose a reason for hiding this comment

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

LGTM

We can remove this warning in another PR if it's necessary

@roboquat roboquat merged commit 3ecb0f7 into main Apr 6, 2022
@roboquat roboquat deleted the mp/local-app-tweaks branch April 6, 2022 08:30
@roboquat roboquat added deployed: IDE IDE change is running in production deployed Change is completely running in production labels Apr 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployed: IDE IDE change is running in production deployed Change is completely running in production release-note-none size/L team: IDE
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants