Re-writes activations logs command subtree in Go #1282
Conversation
…e-merge-main Merge main into feature/serverless
…alocean#1232) * Add support for triggers * Add lastRun field to trigger list output * Hide commands we won't be supporting in EA day 1 * Bump deployer version to pick up bug fix * Fix error handling in services related to triggers Many calls were not checking for errors. * Switch to latest API Change both the triggers command (native to doctl) and the deployer version (which affects the semantics of deploy/undeploy). * Pick up latest deployer (triggers bug fix) * Remove support for prototype API and clean up code * Fix unit tests * Fix misleading comment * Remove added complexity due to successive change * Add filtering by function when listing triggers * Fix omitted code in DeleteTrigger * Guard triggers get/list with status check Otherwise, the credentials read fails with a cryptic error instead of an informative one when you are not connected to a namespace.
…-main Merge latest `main` changes into `feature/serverless`
This completes the elimination of plugin usage in doctl sls fn and the functions.go source file.
I believe this happened in merge conflict resolution during the recent rebase.
Affects what happens when a failure occurs in the middle of deleting functions and triggers together.
…on-list-no-plugin Finish eliminating plugin usage in 'doctl sls functions' + testing improvements
…#1270) * WIP for converting activations to direct OW flows * Finish recoding 'activations get' in native doctl Tests still to come * Convert the support for sls actv result Tests not converted yet * Generate latet mocks * Fix some comments * Use more realistic timestampes * Revise tests for new paths. Still no output check * Tests are now doing meaningful output comparison Fixed some bugs found once tests were really effective
Re-writes activations list command subtree in Go
…ixes bug with package filtering, and fixes the order we display logs
ddebarros
changed the title
ddebarros
changed the title
commands/activations.go
Outdated
| return nil | ||
|
|
||
| } else if followFlag { | ||
| var wg sync.WaitGroup |
There was a problem hiding this comment.
As we discussed, I don't really know about wait groups so my approval of this section is based on a somewhat superficial reading. Do we want to get someone who is more expert to sign off? This does seem to work but I'm also lacking in ideas about how to test it thoroughly.
There was a problem hiding this comment.
I wonder if we might just want to simplify things here and not use the wait group? Wait groups are very useful when you have multiple pieces of work to do in parallel and want to block until they have all completed. In this case, we're only polling for logs. We can remove a bit of complexity by just calling pollActivations and blocking until it completes. It would need to be updated to return an error.
There was a problem hiding this comment.
I made some updates to remove the wait group. I left the other error and signal channel so I can display a cleaner output when users use control+c
without it I get the signal interrupted which I didn't like.
⇒ go run cmd/doctl/main.go sls activations logs --follow
^Csignal: interrupt…, removes both the timeout and only keeps the ticker
There was a problem hiding this comment.
This is looking fine to me now.
The PR is currently targeted to feature/serverless. Its merge target should now be main, since PR #1290 has been merged and we are no longer staging on the feature/serverless branch. I'm approving on the assumption that the base branch will be edited (and tests re-verified) prior to any merge.
ddebarros
dismissed
joshuaauerbachwatson’s stale review
The base branch was changed.
ddebarros
force-pushed
the
activation-logs
branch
from
5a0e14b to
1b9ce32
Compare
There was a problem hiding this comment.
LGTM 👍
I am slightly concerned to see so many commits in the PR, but the actual changed files seem exactly right. Hoping that once we get past the lump created by the recent merge of feature/serverless and the attendant need to rebase, we will stop seeing regurgitated commits and new PRs will be cleaner. I am assuming this will be squashed when merged.
This PR re-write the
doctl sls activations logssubtree in native go-lang. All existing behavior is maintained.