-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Cherry-pick #2490 Make a copy of MachineInfo in GetMachineInfo() to v0.35 #2672
Cherry-pick #2490 Make a copy of MachineInfo in GetMachineInfo() to v0.35 #2672
Conversation
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
Hi @odinuge. Thanks for your PR. I'm waiting for a google member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@@ -177,6 +177,35 @@ func expectManagerWithContainersV2(containers []string, query *info.ContainerInf | |||
return m, infosMap, handlerMap | |||
} | |||
|
|||
func TestMachineInfoReturnValue(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this test from some other PR? or new code? In https://github.com/google/cadvisor/pull/2490/files I only see topology tests extended.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, as you see in the commit log, it is is a separate test to avoid regressions. Open on splitting that to a separate PR or something, as it doesn't matter that much to me. All the tests have changed quite a bit, so it was impossible to cherry-pick back the test from the original PR.
We can just do a simple copy := *m.machineInfo
instead of cherry-picking the commit, but I think @tedyu deserves the credit for the fix.
/ok-to-test |
/retest |
2 similar comments
/retest |
/retest |
It looks like the test won't pass without some changes to the test scripts here. I'm OK with that, and will merge even if the e2e test fails. But I can't merge without the original author's consent. |
Ping @tedyu |
5ecdd66
to
c9b0c29
Compare
Made a new commit with a shallow copy instead, so all cla stuff should be good to go now! /assign @bobbypage |
/test pull-cadvisor-e2e |
hmm, CI is failing with
is this related to change here as mentioned in #2672 (comment)? This test is fine on testgrid though: https://testgrid.k8s.io/sig-node-cadvisor#cadvisor-e2e |
++ git diff --name-only pages |
ah, but this PR didn't update any of assets / pages as I can see... or does it simply need a rebase? |
IIRC, the assets include the year they were built in. It might be from that. E.g. #2370 |
thanks for the tip, Created #2708 to update the asset file which I'm guessing should fix the test failure here. |
actually scratch that, the error looks like from the copyright years as @dashpole mentioned in #2672 (comment). @odinuge Can you run
on your PR? It should update the copyright years and unblock the test:
|
41f9f4f
to
8ddb4e2
Compare
Thanks @odinuge, CI is green now. |
@odinuge I cut a new cadvisor release to include this cherrypick -- https://github.com/google/cadvisor/releases/tag/v0.35.1 Let me know if you need anything else from my side. |
Ref. discussion in k/k here: kubernetes/kubernetes#94176