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

Update README and add help target to make #363

Merged
merged 1 commit into from
Jul 22, 2017
Merged

Update README and add help target to make #363

merged 1 commit into from
Jul 22, 2017

Conversation

jphuynh
Copy link
Contributor

@jphuynh jphuynh commented Jul 20, 2017

Small update to include shellcheck in the README

Signed-off-by: Jean-Pierre Huynh jean-pierre.huynh@ounet.fr

@codecov-io
Copy link

codecov-io commented Jul 20, 2017 •
edited
Loading

Codecov Report

Merging #363 into master will decrease coverage by 0.69%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master     #363     +/-   ##
=========================================
- Coverage   46.15%   45.45%   -0.7%     
=========================================
  Files         193      193             
  Lines       16069    16057     -12     
=========================================
- Hits         7416     7299    -117     
- Misses       8267     8380    +113     
+ Partials      386      378      -8

@dnephin
Copy link
Contributor

dnephin commented Jul 20, 2017

We don't include all the available tasks in the README these are just here as an example.

Maybe shellcheck could be added to lint (make -f docker.Makefile lint shellcheck).

We could also add a CONTRIBUTING to document it further

@thaJeztah
Copy link
Member

Perhaps we can add a make help to make the Makefile self-documenting, like we have in https://github.com/moby/moby/blob/master/Makefile#L134

@jphuynh
Copy link
Contributor Author

jphuynh commented Jul 20, 2017

I like the idea @thaJeztah 👍

@jphuynh
Copy link
Contributor Author

jphuynh commented Jul 20, 2017

I gave it a go.

@dnephin I'm not sure about the wording for the watch target. Let me know if it sounds good.

thaJeztah
thaJeztah reviewed Jul 20, 2017
View reviewed changes
README.md Outdated

```
$ make -f docker.Makefile lint
$ make -f docker.Makefile lint shellcheck
Copy link
Member

Choose a reason for hiding this comment

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

We can mention the help make target in the readme (to see a list of all make options, type make ......help)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why didn't think about that? 😅 It makes more sense. Thanks.

@jphuynh jphuynh force-pushed the shellcheckReadme branch from 1705ae7 to 24ae347 Compare July 20, 2017 22:22
@jphuynh jphuynh changed the title Update README.md with shellcheck command Update README and add help target to make Jul 20, 2017
dnephin
dnephin reviewed Jul 21, 2017
View reviewed changes
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.

Thanks, just a couple comments

README.md Outdated
@@ -23,12 +23,18 @@ Build binaries for all supported platforms:
$ make -f docker.Makefile cross
```

Run all linting:
Run all linting and shellcheck validation:
Copy link
Contributor

Choose a reason for hiding this comment

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

this comment doesn't match the code example anymore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops :)

Makefile Outdated
.PHONY: test
test:
test: ## run go test (the "-tags daemon" part is temporary)
Copy link
Contributor

Choose a reason for hiding this comment

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

The note about -tags daemon is more for the reading of the file, not someone running tasks, can we remove that bit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok

@jphuynh
Update README and add help target to make
649a586 
Signed-off-by: Jean-Pierre Huynh <jean-pierre.huynh@ounet.fr>
@jphuynh jphuynh force-pushed the shellcheckReadme branch from 24ae347 to 649a586 Compare July 21, 2017 15:57
dnephin
dnephin approved these changes Jul 21, 2017
View reviewed changes
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

thaJeztah
thaJeztah approved these changes Jul 22, 2017
View reviewed changes
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@thaJeztah thaJeztah merged commit deab50b into docker:master Jul 22, 2017
@GordonTheTurtle GordonTheTurtle added this to the 17.07.0 milestone Jul 22, 2017
@jphuynh jphuynh deleted the shellcheckReadme branch July 22, 2017 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thaJeztah thaJeztah thaJeztah approved these changes

@dnephin dnephin dnephin approved these changes

Assignees
No one assigned
Labels
status/0-triage
Projects
None yet
Milestone
17.07.0
Development

Successfully merging this pull request may close these issues.
5 participants
@jphuynh @codecov-io @dnephin @thaJeztah @GordonTheTurtle