-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
adding optional presubmit verify-shellcheck.sh job #13125
Conversation
Hi @pswica. 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 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. |
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.
Made some suggestions.
Would much prefer a Makefile, with a verify target which runs a set of verify scripts instead.
See here for an example.
This allows us to change the underlying implementation in k/release without having to also issue an update to the test-infra job definition.
@justaugustus thanks a lot for guiding me through this. Your suggestions definitely improve this test. I will try to implement this |
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.
Sorry, one more change (because I was spacing out on the job name).
/ok-to-test |
LGTM label has been added. Git tree hash: c839393a12237e917098de336086046abede28b5
|
/retest |
/retest Review the full test history for this PR. Silence the bot with an |
@pswica -- Add this block under test-infra/testgrid/config.yaml Lines 3107 to 3110 in 5839ecd
|
@justaugustus |
For sure, anytime @pswica! |
LGTM label has been added. Git tree hash: be4590276fbb554b8243a56f4cbb4244afa4373a
|
/assign @spiffxp |
/hold |
Removing the hold on this as kubernetes/release#740 has been merged! 🎉 /hold cancel |
this needs docker in docker enabled, the kubekins image does not have shellcheck (and it probably shouldn't) to do that:
|
@BenTheElder |
/approve |
LGTM label has been added. Git tree hash: abade9b194cbb47e388305f7dbac0bcd19b6cfeb
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: justaugustus, pswica, spiffxp 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 |
@pswica: Updated the
In response to this:
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. |
/sig release
/cc @justaugustus
Continues this PR: kubernetes/release#740
Which addresses this issue: kubernetes/release#726
Specifically, this will run a job that links the shell scripts in kubernetes/release. I will use this test to clean up the shell scripts, and then make it an always-run job