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

Fixes # 76094 - Check the content of WHAT for Makefile's verify target #76253

Merged
merged 1 commit into from Apr 16, 2019

Conversation

@YoubingLi
Copy link
Contributor

YoubingLi commented Apr 8, 2019

What type of PR is this?

/kind bug

What this PR does / why we need it:

If the target xx specified in WHAT is not verified, make verify WHAT="xx yy" should fail

Which issue(s) this PR fixes:

Fixes # 76094

Special notes for your reviewer:

/assign @liggitt
/assign @BenTheElder

Does this PR introduce a user-facing change?:

no

NONE
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Apr 8, 2019

Hi @YoubingLi. 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.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

@YoubingLi

This comment has been minimized.

Copy link
Contributor Author

YoubingLi commented Apr 8, 2019

Show resolved Hide resolved hack/make-rules/verify.sh Outdated
@SataQiu

This comment has been minimized.

Copy link
Member

SataQiu commented Apr 8, 2019

/cc @SataQiu

@k8s-ci-robot k8s-ci-robot requested a review from SataQiu Apr 8, 2019

@SataQiu

This comment has been minimized.

Copy link
Member

SataQiu commented Apr 8, 2019

/kind bug
/sig testing
/priority important-soon
/ok-to-test

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Apr 8, 2019

/test pull-kubernetes-godeps

Show resolved Hide resolved hack/make-rules/verify.sh Outdated
@BenTheElder

This comment has been minimized.

Copy link
Member

BenTheElder commented Apr 8, 2019

@YoubingLi I fixed the release note FYI, it needs to be NONE specifically for no release note (we parse this) 😅

@BenTheElder

This comment has been minimized.

Copy link
Member

BenTheElder commented Apr 8, 2019

skimmed this, what does the failure output look like?

@YoubingLi

This comment has been minimized.

Copy link
Contributor Author

YoubingLi commented Apr 9, 2019

skimmed this, what does the failure output look like?

It is similar as case failure

 make verify WHAT="ggg bb"

 FAILED TESTS

 ggg
 bb
make: *** [verify] Error 1
@YoubingLi

This comment has been minimized.

Copy link
Contributor Author

YoubingLi commented Apr 9, 2019

/test pull-kubernetes-godeps

@liggitt
staging-godeps test failure

Seems like it relates to your current project. 
Can you give me a clue to resolve it?

@YoubingLi YoubingLi closed this Apr 9, 2019

@YoubingLi

This comment has been minimized.

Copy link
Contributor Author

YoubingLi commented Apr 9, 2019

/open

@YoubingLi YoubingLi reopened this Apr 9, 2019

@cblecker
Copy link
Member

cblecker left a comment

This looks really great! Thanks for doing this. A few bash nits to fix up.

Show resolved Hide resolved hack/make-rules/verify.sh Outdated
Show resolved Hide resolved hack/make-rules/verify.sh Outdated
Show resolved Hide resolved hack/make-rules/verify.sh Outdated
Show resolved Hide resolved hack/make-rules/verify.sh Outdated
Show resolved Hide resolved hack/make-rules/verify.sh Outdated
@cblecker

This comment has been minimized.

Copy link
Member

cblecker commented Apr 9, 2019

@YoubingLi The failure in the pull-kubernetes-godeps job is legitimate.. it's your changes picking up a verify test that the job is attempting, but doesn't exist anymore. Should be fixed when kubernetes/test-infra#12104 merges.

@YoubingLi YoubingLi force-pushed the YoubingLi:bugfix branch from e06db62 to 6fe3cab Apr 9, 2019

@YoubingLi YoubingLi force-pushed the YoubingLi:bugfix branch from 6fe3cab to 8a56ebe Apr 9, 2019

@YoubingLi

This comment has been minimized.

Copy link
Contributor Author

YoubingLi commented Apr 9, 2019

When WHAT is not specified, there is code error

    IFS=" " read -r -a TARGET_LIST <<< "${WHAT}"

It should be
IFS=" " read -r -a TARGET_LIST <<< "${WHAT:-}"

Meanwhile, the number of element in TARGET_LIST should be check in missing-target-checks

@YoubingLi

This comment has been minimized.

Copy link
Contributor Author

YoubingLi commented Apr 9, 2019

/test pull-kubernetes-e2e-gce-100-performance

@cblecker

This comment has been minimized.

Copy link
Member

cblecker commented Apr 9, 2019

/test pull-kubernetes-godeps

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Apr 9, 2019

@YoubingLi: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-godeps 8a56ebe link /test pull-kubernetes-godeps

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@YoubingLi

This comment has been minimized.

Copy link
Contributor Author

YoubingLi commented Apr 9, 2019

/test pull-kubernetes-godeps

@cblecker

verify-godep-licenses.sh and verify-godeps.sh are successed.
The test complains "staging-godeps"

kubernetes/test-infra#12104 should fix it, right?

    FAILED TESTS
    staging-godeps
    Makefile:128: recipe for target 'verify' failed
    make: *** [verify] Error 1
@cblecker

This comment has been minimized.

Copy link
Member

cblecker commented Apr 9, 2019

/lgtm
/approve
/hold

Yes, that PR should fix that job. Going to place hold on this until that's merged and we can check.

@cblecker

This comment has been minimized.

Copy link
Member

cblecker commented Apr 15, 2019

/test pull-kubernetes-dependencies

@cblecker
Copy link
Member

cblecker left a comment

/lgtm
/approve

Thanks for this, @YoubingLi!

@cblecker

This comment has been minimized.

Copy link
Member

cblecker commented Apr 15, 2019

/hold cancel

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Apr 15, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cblecker, YoubingLi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 92562d5 into kubernetes:master Apr 16, 2019

18 checks passed

cla/linuxfoundation YoubingLi authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-godeps Context retired. Status moved to "pull-kubernetes-dependencies".
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.