Conversation
…, delete, and uninstall
mwbrooks
added
code health
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #364 +/- ##
==========================================
- Coverage 65.14% 65.09% -0.06%
==========================================
Files 216 216
Lines 18237 18082 -155
==========================================
- Hits 11881 11770 -111
+ Misses 5259 5222 -37
+ Partials 1097 1090 -7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mwbrooks
left a comment
mwbrooks
left a comment
There was a problem hiding this comment.
Leaving a few comments for the patience reviewers!
| // Set up event logger and execute the command | ||
| log := newDeleteLogger(clients, cmd, team) | ||
| log.Data["appID"] = selection.App.AppID | ||
| env, err := apps.Delete(ctx, clients, log, team, selection.App, selection.Auth) | ||
|
|
||
| return env, err | ||
| } | ||
| // Execute the command | ||
| env, teamName, err := apps.Delete(ctx, clients, team, selection.App, selection.Auth) | ||
| if err != nil { | ||
| return env, err | ||
| } | ||
| printDeleteApp(ctx, clients, selection.App.AppID, teamName) | ||
|
|
||
| // newDeleteLogger creates a logger instance to receive event notifications | ||
| func newDeleteLogger(clients *shared.ClientFactory, cmd *cobra.Command, envName string) *logger.Logger { | ||
| ctx := cmd.Context() | ||
| return logger.New( | ||
| // OnEvent | ||
| func(event *logger.LogEvent) { | ||
| appID := event.DataToString("appID") | ||
| teamName := event.DataToString("teamName") | ||
| switch event.Name { | ||
| case "on_apps_delete_app_success": | ||
| printDeleteApp(ctx, clients, appID, teamName) | ||
| default: | ||
| // Ignore the event | ||
| } | ||
| }, | ||
| ) |
There was a problem hiding this comment.
note: This is a good example of a typical change.
There was a problem hiding this comment.
🌟 praise: It's taken me a while to grasp this logging pattern. Thanks so much for making it more simple and consistent!
|
|
||
| logger := newValidateLogger(clients, cmd) | ||
| log, warn, err := manifestValidateFunc(ctx, clients, logger, selection.App, token) | ||
| isValid, warn, err := manifestValidateFunc(ctx, clients, selection.App, token) |
There was a problem hiding this comment.
note: We now return isValid instead of log
There was a problem hiding this comment.
💰 praise: I like how this makes later logic more obvious!
| return types.App{}, "", slackerror.Wrap(err, slackerror.ErrAppRemove) | ||
| } | ||
|
|
||
| // Is this needed anymore? |
There was a problem hiding this comment.
note: The caller may not have access to the team name and only the team domain or team ID.
There was a problem hiding this comment.
🔏 thought: We should perhaps keep revisiting how selections are kept throughout a command! I understand the app might not exist after deletion but we should be confident in having the selection details kept!
🗣️ ramble(non-blocking): I'm most curious if teamName can be removed from the return arguments.
There was a problem hiding this comment.
Yea, I agree, I'd love to revisit how selections are kept throughout the command. Perhaps slackcontext is our answer?
There was a problem hiding this comment.
I may be overlooking something, but we need the teamName for output purposes but the caller only has the team domain or team ID from the selection. We could move the output into this internal/ package, but I'd rather not do that in this PR. I'm trying to shuffle as little around as possible.
zimeg
left a comment
zimeg
left a comment
There was a problem hiding this comment.
@mwbrooks Super big fan of these changes! Thanks for making the codebase more simple for upcoming efforts 👾 ✨
I left comments and praise with nothing blocking this from merging. The inlined outputs make API calls much simple to reason about for me 🧠 🙏
| _ = printBulkDeleteResult(clients, cmd, result) | ||
| printDatastoreBulkDeleteSuccess(cmd) |
There was a problem hiding this comment.
🌚 question(non-blocking): Do you think outputs will be joined in later changes? No rush at all, but this still seems indirect to me!
There was a problem hiding this comment.
I definitely think we should take a step back and re-consider our outputs. We have outputs mixed between cmd/ and internal/pkg. My ideal world (right now) is that we 🪓 axe internal/pkg and move it all into cmd/. It'll make each command longer but keep it as "single file" and remove the questions of where things should go (cmd vs internal/pkg).
| // Set up event logger and execute the command | ||
| log := newDeleteLogger(clients, cmd, team) | ||
| log.Data["appID"] = selection.App.AppID | ||
| env, err := apps.Delete(ctx, clients, log, team, selection.App, selection.Auth) | ||
|
|
||
| return env, err | ||
| } | ||
| // Execute the command | ||
| env, teamName, err := apps.Delete(ctx, clients, team, selection.App, selection.Auth) | ||
| if err != nil { | ||
| return env, err | ||
| } | ||
| printDeleteApp(ctx, clients, selection.App.AppID, teamName) | ||
|
|
||
| // newDeleteLogger creates a logger instance to receive event notifications | ||
| func newDeleteLogger(clients *shared.ClientFactory, cmd *cobra.Command, envName string) *logger.Logger { | ||
| ctx := cmd.Context() | ||
| return logger.New( | ||
| // OnEvent | ||
| func(event *logger.LogEvent) { | ||
| appID := event.DataToString("appID") | ||
| teamName := event.DataToString("teamName") | ||
| switch event.Name { | ||
| case "on_apps_delete_app_success": | ||
| printDeleteApp(ctx, clients, appID, teamName) | ||
| default: | ||
| // Ignore the event | ||
| } | ||
| }, | ||
| ) |
There was a problem hiding this comment.
🌟 praise: It's taken me a while to grasp this logging pattern. Thanks so much for making it more simple and consistent!
|
|
||
| logger := newValidateLogger(clients, cmd) | ||
| log, warn, err := manifestValidateFunc(ctx, clients, logger, selection.App, token) | ||
| isValid, warn, err := manifestValidateFunc(ctx, clients, selection.App, token) |
There was a problem hiding this comment.
💰 praise: I like how this makes later logic more obvious!
| return types.App{}, "", slackerror.Wrap(err, slackerror.ErrAppRemove) | ||
| } | ||
|
|
||
| // Is this needed anymore? |
There was a problem hiding this comment.
🔏 thought: We should perhaps keep revisiting how selections are kept throughout a command! I understand the app might not exist after deletion but we should be confident in having the selection details kept!
🗣️ ramble(non-blocking): I'm most curious if teamName can be removed from the return arguments.
|
@zimeg Thanks for the review! 🙇🏻 It's a gnarly PR and I appreciate that you've taken the time to really look it over and think about the changes. Your questions have a lot of weight and value. My hope is that by removing the For this PR, I'd feel most comfortable keeping it scoped to surgically removing It'll feel good to merge these PRs and give them a few days to bake 🍞 before our next release! |
|
@mwbrooks Super appreciate the responses too! I realize you might be thinking the same things while knowing how to scope a change ✨ 🔭 |
2 participants
Changelog
Summary
This pull request is part 1 of removing
internal/loggerpackage and is focused on the straight-forward use-cases. It will be the largest, but simplest PR.internal/loggerdependency from 5 command groups (activity,manifest,datastore,auth, andapp delete/uninstall)*logger.LogEvent, with output printed directly by callersMotivation
The
internal/loggerpackage implements an event-driven pub-sub pattern whereinternal/pkg/emits named events through a logger andcmd/subscribes to those events with callbacks that print output. This indirection makes control flow harder to follow and has already been abandoned in newer commands (e.g.cmd/datastore/count.go) which return plain data and print directly.Test Steps
The ambitious can double-check the output against their production
slackinstallation.Requirements