-
Notifications
You must be signed in to change notification settings - Fork 15
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
feat(BUX-420): go version and workflows update and config fix for k8s #390
Conversation
Welcome to our open-source project @kuba-4chain! π |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #390 +/- ##
==========================================
+ Coverage 39.16% 39.36% +0.19%
==========================================
Files 44 44
Lines 1560 1570 +10
==========================================
+ Hits 611 618 +7
- Misses 917 918 +1
- Partials 32 34 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can live with that, but I just want to mention that it isn't best practice to mix bumping go version and changes in config in one PR.
cfg, err := json.MarshalIndent(appConfig, "", " ") | ||
if err != nil { | ||
logger.Error().Msg("Unable to decode App Config to json") | ||
} else { | ||
fmt.Printf("loaded config: %s", cfg) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- as we have a struct with config, why don't you just use
Printf("loaded config: %v", appConfig)
? - Why you're doing fmt.Printf instead of using a logger?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Because it will print it in one-liner.
- Because it will print it in one-liner π
Doing it this way, we print it as a formatted json which makes it MUCH easier to look at quickly and seeing if everything is okay. This is also shown only when when debug
flag is selected. That's why I decided to make an exception here and use the standard fmt.Printf function.
β¦lisions with k8s
812a35e
to
daec619
Compare
Pull Request Checklist