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

Fix preflight test error message #791

Merged

Conversation

waynr
Copy link
Contributor

@waynr waynr commented Jul 2, 2019

What this PR does / why we need it:

It fixes a confusing error message in the preflight checks. It's not strictly
needed, but it is nice as a user when error messages make sense.

Which issue(s) this PR fixes

There is currently no issue for this problem.

Special notes for your reviewer:

Release note:

Fix dns preflight test error message

@johnSchnake
Copy link
Contributor

💯 That is definitely confusing.

Bonus points if you're interested in expanding it to explain to the user what labels it was looking for/where. Although the message would now be more clear, I can imagine they'd immediately know what Sonobuoy considers a "dns pod" especially if they are experimenting with their own deployment or dns solution.

@johnSchnake johnSchnake added the area/documentation Related to project documentation label Jul 2, 2019
@johnSchnake johnSchnake self-assigned this Jul 2, 2019
@waynr waynr force-pushed the fix-dns-preflight-test-error-message branch 2 times, most recently from 60e0277 to 5387355 Compare July 2, 2019 20:04
@johnSchnake
Copy link
Contributor

CI will fail with this:

--- FAIL: TestDNSCheck (0.00s)
    --- FAIL: TestDNSCheck/Requires_at_least_one_pod (0.00s)
        preflight_test.go:168: Expected error to be "no dns pod tests found" but got "no dns pods found with the labels [foo] in namespace kube-system"
FAIL

I think everything else should be fine. After you push the unit test fix and CI is green I'll merge.

Thanks for the contribution!

@johnSchnake
Copy link
Contributor

Sorry for the miscue; that second commit should be squashed into the first so that it is signed as well, otherwise the sign off check fails.

Signed-off-by: Wayne Warren <wayne.warren.s@gmail.com>
@waynr waynr force-pushed the fix-dns-preflight-test-error-message branch from ab1d92b to 6aa7d50 Compare July 2, 2019 22:13
@johnSchnake
Copy link
Contributor

💯 Thanks for the quick iterations on those. Merging since I validated the unit test signal locally.

@johnSchnake johnSchnake merged commit c029d9f into vmware-tanzu:master Jul 2, 2019
@codecov-io
Copy link

Codecov Report

Merging #791 into master will increase coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #791      +/-   ##
==========================================
+ Coverage   42.59%   42.64%   +0.04%     
==========================================
  Files          71       71              
  Lines        4193     4193              
==========================================
+ Hits         1786     1788       +2     
+ Misses       2300     2298       -2     
  Partials      107      107
Impacted Files Coverage Δ
pkg/client/preflight.go 57.35% <100%> (ø) ⬆️
cmd/sonobuoy/app/status.go 59.34% <0%> (+2.19%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dca7c17...6aa7d50. Read the comment docs.

@waynr
Copy link
Contributor Author

waynr commented Jul 3, 2019

@johnSchnake thanks for the quick reviews!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/documentation Related to project documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants