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

Add make verify target #13

Merged
merged 1 commit into from
Sep 26, 2018
Merged

Conversation

twiest
Copy link
Contributor

@twiest twiest commented Sep 24, 2018

  • Add make verify target
  • Add verify-lint target
  • Add verify-gofmt target
  • Add verify-go-vet target
  • Fix make verify-imports target to error when verify-imports fails.
  • Fix golint errors in codebase
  • Remove fmt and vet targets from build targets (necessary for make verify to work)
  • Add .PHONY to Makefile targets that don't produce files of the same name.

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 24, 2018
@dgoodwin
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 25, 2018
@twiest

This comment has been minimized.

@twiest

This comment has been minimized.

@twiest
Copy link
Contributor Author

twiest commented Sep 25, 2018

/shrug

- [x] Add make verify target
- [x] Add verify-lint target
- [x] Add verify-gofmt target
- [x] Add verify-go-vet target
- [x] Fix make verify-imports target to error when verify-imports fails.
- [x] Fix golint errors in codebase
- [x] Remove fmt and vet targets from build targets (necessary for make verify to work)
- [x] Add .PHONY to Makefile targets that don't produce files of the same name.
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 25, 2018
@twiest
Copy link
Contributor Author

twiest commented Sep 25, 2018

@dgoodwin hey, I rebased on master and had to update some of the new code to comply with go vet's checks.

Would you mind reviewing this again please?

@dgoodwin
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 25, 2018
@openshift-merge-robot openshift-merge-robot merged commit 3adb68d into openshift:master Sep 26, 2018
wking added a commit to wking/openshift-installer that referenced this pull request Sep 29, 2018
I updated the import path to keep up with
openshift/hive@ae569e39 (Add make verify target, 2018-09-24,
openshift/hive#13).  Then I bumped vendor/ with:

  $ rm -rf ~/.glide/cache/info/https-github.com-openshift-hive.json ~/.glide/cache/src/https-github.com-openshift-hive/
  $ glide remove github.com/openshift/hive
  $ glide get --strip-vendor github.com/openshift/hive
  $ glide-vc --use-lock-file --no-tests --only-code
  $ git checkout HEAD -- vendor/github.com/shurcooL/httpfs

using:

  $ glide --version
  glide version 0.13.2-dev
  $ (cd $GOPATH/src/github.com/Masterminds/glide && git describe)
  v0.13.1-7-g3e13fd1
  $ (cd $GOPATH/src/github.com/sgotti/glide-vc && git describe)
  v0.1.0-2-g6ddf6ee

I don't	know why I needed to clear my cache, but without that
line the remaining commands didn't bump
vendor/github.com/openshift/hive to its current master.

Bumping to openshift/hive@a050d775 (Merge pull request
openshift/hive#28 from wking/tag-20-load-balancers-per-request,
2018-09-28) gives us more reliable load-balancer deletion.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm 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
Development

Successfully merging this pull request may close these issues.

4 participants