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 scripts and targets for manpages and yamldocs #154

Merged
merged 1 commit into from
Jun 5, 2017

Conversation

tiborvass
Copy link
Collaborator

Signed-off-by: Tibor Vass tibor@docker.com

Makefile Outdated
@@ -3,20 +3,22 @@
#
all: binary

PKGS := $(shell go list ./... | grep -vE '/vendor/|github.com/docker/cli/man|github.com/docker/cli/docs/yaml$$')
Copy link
Contributor

@dnephin dnephin Jun 5, 2017

Choose a reason for hiding this comment

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

This is a problem. It causes a delay on every run of make even if we don't use this list.

I think this needs to be inside the scripts that use it (like it was previously).

This is unrelated to the docs generation scripts, right ? So at the very least could be discussed in a separate PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to exclude man/ or docs/ from unit testing anymore , now that we have all the dependencies installed in vendor/.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@dnephin sure, I will simply duplicate it in this Makefile, but I do want to remove grep -v vendor in unit-test-with-coverage, and replace it with $@.

@@ -0,0 +1,5 @@
#!/bin/sh
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be moved to scripts/docs/generate-yaml.sh to be consistent with our other scripts?

@@ -7,6 +7,16 @@ set -eu

mkdir -p ./man/man1

MD2MAN_REPO=github.com/cpuguy83/go-md2man
Copy link
Contributor

Choose a reason for hiding this comment

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

And this could be scripts/docs/generate-man.sh to be consistent with our other scripts.

@codecov-io
Copy link

codecov-io commented Jun 5, 2017

Codecov Report

Merging #154 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #154   +/-   ##
=======================================
  Coverage   44.96%   44.96%           
=======================================
  Files         169      169           
  Lines       11381    11381           
=======================================
  Hits         5117     5117           
  Misses       5971     5971           
  Partials      293      293

Makefile Outdated

# run go test
# the "-tags daemon" part is temporary
.PHONY: test
test:
./scripts/test/unit $(shell go list ./... | grep -v /vendor/)
./scripts/test/unit $(shell go list ./... | grep -vE '/vendor/|github.com/docker/cli/man|github.com/docker/cli/docs/yaml$$')
Copy link
Contributor

Choose a reason for hiding this comment

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

This no longer needs to ignore docs/ or man/

Signed-off-by: Tibor Vass <tibor@docker.com>
Copy link
Contributor

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Collaborator

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🐯

@tiborvass tiborvass merged commit ed5b663 into docker:master Jun 5, 2017
@GordonTheTurtle GordonTheTurtle added this to the 17.06.0 milestone Jun 5, 2017
@mavenugo mavenugo mentioned this pull request Jun 6, 2017
23 tasks
@tiborvass tiborvass mentioned this pull request Jun 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants