Closed G-Harmon closed 6 years ago
Hi @G-Harmon. Thanks for your PR.
I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test
on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.
I understand the commands that are listed here.
/ok-to-test
On Nov 22, 2017 11:35 AM, "k8s-ci-robot" notifications@github.com wrote:
Hi @G-Harmon https://github.com/g-harmon. Thanks for your PR.
I'm waiting for a kubernetes https://github.com/orgs/kubernetes/people member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.
I understand the commands that are listed here https://github.com/kubernetes/test-infra/blob/master/commands.md.
Instructions for interacting with me using PR comments are available here https://github.com/kubernetes/community/blob/master/contributors/devel/pull-requests.md. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue: repository.
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/GoogleCloudPlatform/k8s-multicluster-ingress/pull/78#issuecomment-346452680, or mute the thread https://github.com/notifications/unsubscribe-auth/AJugO_71sVGBkfldWjUKk7hcTkWKuhA4ks5s5Hd3gaJpZM4Qn57H .
all right, coverage was posted here! PTAL. cc @nikhiljindal @BenTheElder @csbell
hmm good that it is posting. I assume 60% is the coverage with this PR and not the current coverage in our repo. Can you verify that?
Also, not sure why does it say "Changes unknown"?
Not sure about the changes unknown but these look like the correct env skimming the coveralls docs. The coverage report should be this PR against master which doesn't change any go code so... coverage against master?
I pushed a version of the commit where i moved some testes. Instead of 60%, it shows 53%, so it IS displaying coverage with the current PR. It still doesn't have the "change" though :-/
I am fine with merging this PR while we debug the Changes unknown thing. @G-Harmon can you revert the test changes?
okay, cool. I've pushed the original commit again. I think it's ready to be merged.
…comments back to PR.
I think the best way to test this out is to put up this PR and see if it gets a coverage report. The firewallrulesyncer.go change is just to make sure coveralls sees some Golang diff. I don't want to submit it.
This change is