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

Refactor hooks, add opt-in pre-push hook. #363

Merged
merged 1 commit into from
Jul 6, 2015
Merged

Refactor hooks, add opt-in pre-push hook. #363

merged 1 commit into from
Jul 6, 2015

Conversation

klausw
Copy link
Contributor

@klausw klausw commented Jul 6, 2015

See hooks/README.md for implementation notes.

Motivtion: the old hooks are hard to run manually, and code reuse among
hooks is difficult. Goal of this refactor is to cleanly split the steps
involved:

  • determine set of changed files
  • run checks on specific files
  • report issues based on check results

The old logic was based on running checks on individual files, printing
"1" on STDOUT for bad files. Now, check-* scripts take a list of files
as args and print filenames of bad files on STDOUT. A toplevel "check"
script drives these individual checks and contains common hook logic
such as problem reporting. The toplevel hooks themselves use the
"check" script to take appropriate actions.

klausw
klausw reviewed Jul 6, 2015
View reviewed changes
hooks/lib/check-boilerplate.sh

# Treat errors from diff other than the usual "files differ"
# code 1 as fatal problems.
DIFFER=$(echo "${HEADER}" | diff - "${REF_FILE}" || [[ $? -le 1 ]])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI, the content of this file is mostly just copied from the previous boilerplate.sh file, replacing "echo 0" with "continue". I added the diff exit code check here since the original wasn't using strict -euo pipefail error checking.

@cmccoy
Copy link
Contributor

cmccoy commented Jul 6, 2015

Regarding the pre-push hook: how about PKB_BASE instead of PKB_ORIGIN? origin is usually the name of a remote, but the hook requires a revision.

@klausw klausw force-pushed the dev-klausw-pre-push-hook branch 2 times, most recently from 5372a94 to c35e323 Compare July 6, 2015 20:29
@klausw
Copy link
Contributor Author

klausw commented Jul 6, 2015

Changed to use PKB_BASE.

On Mon, Jul 6, 2015 at 1:11 PM, Connor McCoy [email protected]
wrote:

Regarding the pre-push hook: how about PKB_BASE instead of PKB_ORIGIN?
origin is usually the name of a remote, but the hook requires a revision.

—
Reply to this email directly or view it on GitHub
#363 (comment)
.

@klausw klausw force-pushed the dev-klausw-pre-push-hook branch 4 times, most recently from bcbea2e to df987cc Compare July 6, 2015 20:56
@klausw
Copy link
Contributor Author

klausw commented Jul 6, 2015

PTAL - I've changed the error handling for the prepare-commit-msg and check scripts so that they can use a nonzero exit code iff there are unexpected errors that weren't covered by the text report.

cmccoy
cmccoy reviewed Jul 6, 2015
View reviewed changes
hooks/check
for file in "${files_need_boilerplate[@]}"; do
echo " ${file}"
done
echo "See hooks/boilerplate.*.txt for required headers."
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be: hooks/lib/boilerplate.*.txt.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@cmccoy
Copy link
Contributor

cmccoy commented Jul 6, 2015

One more small thing, otherwise LGTM. Thanks for fixing this up!

@cmccoy cmccoy assigned klausw and unassigned cmccoy Jul 6, 2015
@klausw
Refactor hooks, add opt-in pre-push hook.
d6c5761 
See hooks/README.md for implementation notes.

Motivtion: the old hooks are hard to run manually, and code reuse among
hooks is difficult. Goal of this refactor is to cleanly split the steps
involved:

- determine set of changed files
- run checks on specific files
- report issues based on check results

The old logic was based on running checks on individual files, printing
"1" on STDOUT for bad files. Now, check-* scripts take a list of files
as args and print filenames of bad files on STDOUT. A toplevel "check"
script drives these individual checks and contains common hook logic
such as problem reporting.  The toplevel hooks themselves use the
"check" script to take appropriate actions.

Also change checks to use strict `-euo pipefail` error handling where
possible, and standardize variable naming. (Uppercase for top-of-file
constants or vars exported to the environment, otherwise lowercase.)
klausw added a commit that referenced this pull request Jul 6, 2015
@klausw
Merge pull request #363 from GoogleCloudPlatform/dev-klausw-pre-push-…
6672b60 
…hook

Refactor hooks, add opt-in pre-push hook.
@klausw klausw merged commit 6672b60 into dev Jul 6, 2015
@klausw klausw deleted the dev-klausw-pre-push-hook branch July 6, 2015 21:48
@klausw klausw mentioned this pull request Jul 6, 2015
Merged
klausw added a commit that referenced this pull request Jul 9, 2015
@klausw
Merge tag 'v0.18.0'
f593091 
Release 0.18.0.

(See also #369
which includes this change log with clickable GH-* links.)

* New features:
  * Support OpenStack as cloud provider (GH-305, GH-353, thanks @kivio and
    @mateusz-blaszkowski)
  * Support Rackspace as cloud provider (GH-336, thanks @meteorfox and @jrperritt)
  * Add support for ContainerizedVM using docker exec (GH-333, thanks @gablg1)
  * Windows guest VM support on Static VM (GH-350), Azure (GH-349, GH-374), AWS
    (GH-347), and GCE (GH-338)
  * Add NTttcp Windows networking benchmark (GH-348)

* Enhancements:
  * Support using proxies in VMs (GH-339, GH-337, thanks @kivio)
  * Enable optional migration on GCE (GH-343)
  * Execute long running commands via a remote agent (GH-310)
  * Add resource creation/deletion times to logs (GH-316)

* Bugfixes and maintenance updates:
  * Update PKB to work with Azure version 0.9.3 (GH-312)
  * Fix AWS CLI usage on Windows host (GH-313)
  * Auto-fetch AMI IDs for AWS images (GH-364)
  * Fix publisher missing info for default image and machine type (GH-357)
  * Fix 'no attribute pkb_thread_log_context' error for sub-thread logs (GH-322)

* Benchmark-specific changes:
  * aerospike: config/flag handling bugfixes (GH-367, GH-360, GH-354)
  * cassandra_ycsb: move num_vms prerequisite check
  * fio: add latency percentiles for results (GH-344)
  * hadoop_terasort: Fix bad SSH option (GH-328)
  * iperf: add lower bounds to arguments (GH-314)
  * iperf: add timeout to parallel benchmark runs to handle iperf hangs (GH-375)
  * netperf: Support confidence intervals, increase test length, report stddev
    (GH-317, GH-306)
  * ycsb: Drop unaggregatable results from samples (GH-324)

* Development and testing:
  * **Breaking Change** Automated testing now uses `tox` (GH-330)
  * Refactored hook scripts, including new opt-in pre-push hook (GH-363)
  * Use travis for CI testing (GH-340)
  * Speed up tests using timeouts (GH-299)

* Internals:
  * Move defaults from benchmark_spec to VM classes, move network instantiation
    out of benchmark spec (GH-342)
  * Add event hook support (GH-315)
  * Refactor VM classes (GH-321)
@klausw klausw mentioned this pull request Jul 9, 2015
Closed
@cmccoy cmccoy mentioned this pull request Jul 10, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@klausw klausw

Labels
LGTM
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@klausw @cmccoy