Remove use of global sonobuoy pod name #811
Conversation
Codecov Report
@@ Coverage Diff @@
## master #811 +/- ##
=========================================
Coverage ? 43.42%
=========================================
Files ? 71
Lines ? 4260
Branches ? 0
=========================================
Hits ? 1850
Misses ? 2301
Partials ? 109
Continue to review full report at Codecov.
|
pkg/discovery/discovery.go
Outdated
| // Determine sonobuoy pod name | ||
| podName, err := pluginaggregation.GetStatusPodName(client, namespace) | ||
| if err != nil { | ||
| return errors.Wrap(err, "failed to get name of status pod") |
There was a problem hiding this comment.
status pod I think is a little confusing.
Suggestion failed to get the name of the aggregator pod to set the status on
There was a problem hiding this comment.
There are a few other lines like this that I stopped commenting it on; fyi (so you dont just look at the lines I pointed out)
pkg/plugin/aggregation/status.go
Outdated
| SetStatusPodName(client, namespace) | ||
| podName, err := GetStatusPodName(client, namespace) | ||
| if err != nil { | ||
| return nil, errors.Wrap(err, "failed to get name of status pod") |
pkg/plugin/aggregation/update.go
Outdated
| listOptions := metav1.ListOptions{ | ||
| LabelSelector: StatusPodLabel, | ||
| } | ||
|
|
||
| podList, err := client.CoreV1().Pods(namespace).List(listOptions) | ||
| if err != nil { | ||
| logrus.Errorf("Error listing pods with label '%s': %s", StatusPodLabel, err) | ||
| return | ||
| return "", errors.Wrap(err, "Error listing pods with label %q") |
There was a problem hiding this comment.
Should wrap with lower case phrases; only start with a capital when logging since you know it wont be further wrapped/prefixed.
There was a problem hiding this comment.
Thanks for pointing that out. I had taken this from the previous log line and forgot to update that.
| @@ -47,3 +54,75 @@ func TestCreateUpdater(t *testing.T) { | |||
| t.Errorf("expected status to be failed, got %v", updater.status.Status) | |||
| } | |||
| } | |||
|
|
|||
| func TestGetStatusPodName(t *testing.T) { | |||
There was a problem hiding this comment.
👍 having the test. I just talked to someone who was suggesting we tweak the logic this method would use to take into account pod status (filter to only consider running pods). Something to consider, but regardless of the decision there, having the test will help us ensure that functionality.
There was a problem hiding this comment.
I'm happy to make that change but would prefer to do it in a follow up branch just to avoid adding another change in functionality here.
There was a problem hiding this comment.
Oh for sure; I didnt mean to do that here. Was just adding context for why I like having this test now (that it will immediately be useful for testing/adding requested functionality)
zubron
force-pushed
the
remove-use-of-status-pod-name
branch
from
1ecd350 to
fd1215e
Compare
Previously the sonobuoy pod name was stored in a global variable and was updated before it was used. Now, instead of updating the global variable the function that determines the pod name returns it instead. Signed-off-by: Bridget McErlean <[email protected]>
zubron
force-pushed
the
remove-use-of-status-pod-name
branch
from
fd1215e to
9971fbb
Compare
What this PR does / why we need it:
Previously the sonobuoy pod name was stored in a global variable and was
updated before it was used. Now, instead of updating the global variable
the function that determines the pod name returns it instead.
Signed-off-by: Bridget McErlean [email protected]
Special notes for your reviewer:
Release note: