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.

Sign up for GitHub

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

Jump to bottom

rename verify/ to hack/ for consistency with k/k #5199

Merged
merged 1 commit into from
Nov 2, 2017
Merged

rename verify/ to hack/ for consistency with k/k #5199

merged 1 commit into from
Nov 2, 2017

Conversation

BenTheElder
Copy link
Member

@BenTheElder BenTheElder commented Oct 26, 2017 •
edited
Loading

fixes #5011, also adds hack/update-all.sh

now you can run hack/update-all.sh e̶v̶e̶r̶y̶w̶h̶e̶r̶e̶ in kubernetes and test-infra! 😆

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 26, 2017
@BenTheElder
Copy link
Member Author

/assign @Kargakis @ixdy @krzyzacy

@BenTheElder
Copy link
Member Author

/area bikeshed
😂

@BenTheElder
Copy link
Member Author

/shrug
/joke

@k8s-ci-robot
Copy link
Contributor

@BenTheElder: Which side of the chicken has more feathers? The outside.

In response to this:

/shrug
/joke

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.

@k8s-ci-robot k8s-ci-robot added the ¯\_(ツ)_/¯ ¯\\\_(ツ)_/¯ label Oct 26, 2017
@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 26, 2017
@BenTheElder
Copy link
Member Author

These are all failing since they try to run the script from the old location 🤷‍♂️

@krzyzacy
Copy link
Member

seems now we have hack/foo.sh and hack/verify/foo.sh now
otherwise it's fine

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 27, 2017
@BenTheElder
Copy link
Member Author

seems now we have hack/foo.sh and hack/verify/foo.sh now otherwise it's fine

I really don't know how I managed to do that, it should be fixed now.

@BenTheElder
Copy link
Member Author

I should add a hack/update-all.sh so you can run that in k/k or k/t-i and know your branch is well-formatted 🤔

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 27, 2017
@BenTheElder
Copy link
Member Author

Added hack/update-all.sh.

@cblecker
Copy link
Member

cblecker commented Oct 27, 2017 •
edited
Loading

/area bikeshed

Can this be an alias for /shrug? 😂

@BenTheElder
Copy link
Member Author

Can this be an alias for /shrug? 😂

SGTM, @krzyzacy 😄 ?

0xmichalis
0xmichalis reviewed Oct 30, 2017
View reviewed changes
hack/update-all.sh

TESTINFRA_ROOT=$(git rev-parse --show-toplevel)
${TESTINFRA_ROOT}/hack/update-bazel.sh
${TESTINFRA_ROOT}/hack/update-gofmt.sh
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is update-gofmt.sh mutating the actual files or is it sending its output to stdout? If the latter, I am not sure how useful it is. Maybe a follow-up issue to fix it to actually update.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's mutating them if they need formatting (it has the -w flag).

@BenTheElder
Copy link
Member Author

Anyone? I'm often typing hack/update-bazel.sh after switching between k/k and k/t-i and it seems we need to keep these more in sync.

@krzyzacy
Copy link
Member

krzyzacy commented Nov 2, 2017

/lgtm
/hold
manually merge and watch the 🔥 tomorrow?
bazel-test-affected.sh might already be a dead script

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Nov 2, 2017
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder, krzyzacy

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

Needs approval from an approver in each of these OWNERS Files:
  • OWNERS [BenTheElder,krzyzacy]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@kubernetes kubernetes deleted a comment from k8s-github-robot Nov 2, 2017
@cblecker
Copy link
Member

cblecker commented Nov 2, 2017

This conflicts with #5301.. so one has to go first and the other rebase. I don’t have a preference as to which :)

@BenTheElder
Copy link
Member Author

BenTheElder commented Nov 2, 2017 via email

@BenTheElder
Copy link
Member Author

should be good to go now 🚲 🏚️ 🔥
/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 2, 2017
@BenTheElder BenTheElder merged commit 9f7e205 into kubernetes:master Nov 2, 2017
@k8s-ci-robot
Copy link
Contributor

@BenTheElder: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-test-infra-verify-gofmt 597ae39 link /test pull-test-infra-verify-gofmt
pull-test-infra-verify-bazel 597ae39 link /test pull-test-infra-verify-bazel
pull-test-infra-verify-govet 597ae39 link /test pull-test-infra-verify-govet
pull-test-infra-bazel 597ae39 link /test pull-test-infra-bazel

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.

@BenTheElder BenTheElder mentioned this pull request Nov 2, 2017
Merged
@BenTheElder
Copy link
Member Author

missed the test-infra bazel jobs specifying //verify:verify-all somehow (bad rebase?), fixed in the follow-up.

@BenTheElder BenTheElder deleted the hackity-hack branch November 2, 2017 19:19
@stevekuznetsov
Copy link
Contributor

Would have been nice to symlink or e-mail blast this one :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xmichalis 0xmichalis 0xmichalis left review comments

Assignees

@krzyzacy krzyzacy

@0xmichalis 0xmichalis

@ixdy ixdy

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. ¯\_(ツ)_/¯ ¯\\\_(ツ)_/¯
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Rename /verify directory to /hack or something
8 participants
@BenTheElder @k8s-ci-robot @krzyzacy @cblecker @stevekuznetsov @0xmichalis @ixdy @k8s-github-robot