fix tomlv because it doesn't read from stdin #10872

jessfraz · 2015-02-18T07:57:58Z

No description provided.

LK4D4 · 2015-02-18T18:43:34Z

Hahaha
LGTM

tianon · 2015-02-18T21:58:06Z

project/make/validate-toml

-	if [ "$(git show "$VALIDATE_HEAD:$f" | tomlv)" ]; then
-		badFiles+=( "$f" )
-	fi
+	tomlv $f || badFiles+=( "$f" )


That first $f needs double quotes around it. 😉

Hah, interesting: our comments came back because the same diff as when they were made is back.

(ie, this needs to be git show "$VALIDATE_HEAD:$f" | tomlv /proc/self/fd/0 || badFiles+=("$f"))

(or your expanded if which is probably easier to read and understand)

Ah it's because I was on my other computer facepalm

On Wednesday, February 18, 2015, Tianon Gravi notifications@github.com
wrote:

In project/make/validate-toml
#10872 (comment):

@@ -9,9 +9,7 @@ unset IFS
badFiles=()
for f in "${files[@]}"; do
# we use "git show" here to validate that what's committed is formatted

if [ "$(git show "$VALIDATE_HEAD:$f" | tomlv)" ]; then

badFiles+=( "$f" )

fi

tomlv $f || badFiles+=( "$f" )

(or your expanded if which is probably easier to read and understand)

—
Reply to this email directly or view it on GitHub
https://github.com/docker/docker/pull/10872/files#r24970663.

tianon · 2015-02-18T21:59:47Z

I wonder if it'd work if we passed in /dev/stdin or something like that, so that we could validate the contents of the index directly instead of what's currently on-disk?

jessfraz · 2015-02-18T22:01:18Z

hmm let me try

jessfraz · 2015-02-18T22:05:41Z

nah that doesnt work :(

tianon · 2015-02-18T22:06:01Z

http://nooooooooooooooo.com/

tianon · 2015-02-18T22:09:46Z

Try /pc/self/fd/0 instead.

jessfraz · 2015-02-18T22:13:19Z

Error in '/pc/self/fd/0': open /pc/self/fd/0: no such file or directory

tianon · 2015-02-18T22:14:04Z

oh wtf how did I mistype that, /proc/self/fd/0

tianon · 2015-02-18T22:14:58Z

that's bizarre, because I copy pasted from my terminal

shakes fist at bash TTY handling

jessfraz · 2015-02-18T22:15:54Z

it would help if i used common sense too

tianon · 2015-02-18T22:16:02Z

👍

tianon · 2015-02-18T22:17:32Z

root@d1182ee8fee5:/go/src/github.com/docker/docker# cat MAINTAINERS | tomlv /proc/self/fd/0
Error in '/proc/self/fd/0': Near line 17, key 'Rules.maintainers': Near line 17: Expected a top-level item to end with a new line, comment or EOF, but got '"' instead.

jessfraz · 2015-02-18T22:19:52Z

it works! and updated!

tianon · 2015-02-18T22:19:56Z

LGTM

tianon · 2015-02-18T22:21:22Z

I think we should probably fix the current syntax, shouldn't we?

Error in 'MAINTAINERS': Near line 17, key 'Rules.maintainers': Near line 17: Expected a top-level item to end with a new line, comment or EOF, but got '"' instead.

tianon · 2015-02-18T22:27:16Z

project/make/validate-toml

@@ -9,7 +9,7 @@ unset IFS
 badFiles=()
 for f in "${files[@]}"; do
 	# we use "git show" here to validate that what's committed is formatted
-	if [ "$(git show "$VALIDATE_HEAD:$f" | tomlv)" ]; then
+	if [ "$(git show "$VALIDATE_HEAD:$f" | tomlv /proc/self/fd/0)" ]; then


Actually, this should probably even be checking the return value of tomlv instead, ie:

if ! git show ...... | tomlv ... ]; then

jessfraz · 2015-02-19T00:13:41Z

how can i sed replace stderr because its not working :(

jessfraz · 2015-02-19T00:31:03Z

that is the actual error from tomlv because it thinks /proc/... is the file lol

jessfraz · 2015-02-19T07:19:38Z

Turns out with the version of tomlv we were using we were missing the commits for multiline strings BurntSushi/toml@1956abe

so bumped the version and fixed the current files.

tianon · 2015-02-19T07:54:05Z

Dockerfile

+RUN set -x \
+	&& git clone https://github.com/BurntSushi/toml.git /go/src/github.com/BurntSushi/toml \
+	&& (cd /go/src/github.com/BurntSushi/toml && git checkout -q $TOMLV_COMMIT) \
+	&& go install -v github.com/BurntSushi/toml/cmd/tomlv


I'm not going to let it block this PR, but I've filed BurntSushi/toml#74 to request a new version tag on toml upstream to make sure we're being good citizens. 👍

nice i was going to do that too

jessfraz · 2015-02-19T18:14:51Z

ok updated hopefully for the last time ;)

jessfraz · 2015-02-19T19:43:47Z

@thaJeztah added the GitHub username and realphabetized here :)

thaJeztah · 2015-02-19T19:50:49Z

@jfrazelle are you psychic? I was reading this when your comment popped up 🙀

<----- puts on tinfoil hat

thaJeztah · 2015-02-19T21:12:13Z

MAINTAINERS

+	[people.mary]
+	Name = "Mary Anthony"
+	Email = "mary.anthony@docker.com"
+    GitHub = "moxiegirl"


Mixed tabs/spaces? Indenting looks off

omg my vim config gooooood catch

On Thu, Feb 19, 2015 at 1:12 PM, Sebastiaan van Stijn <
notifications@github.com> wrote:

In MAINTAINERS
#10872 (comment):

@@ -531,6 +531,11 @@ made through a pull request.
Email = "lk4d4@docker.com"
GitHub = "lk4d4"

[people.mary]

Name = "Mary Anthony"

Email = "mary.anthony@docker.com"

GitHub = "moxiegirl"

Mixed tabs/spaces? Indenting looks off

—
Reply to this email directly or view it on GitHub
https://github.com/docker/docker/pull/10872/files#r25025967.

Docker-DCO-1.1-Signed-off-by: Jessica Frazelle <jess@docker.com> (github: jfrazelle) Docker-DCO-1.1-Signed-off-by: Jessie Frazelle <jess@docker.com> (github: jfrazelle)

tianon · 2015-02-20T00:23:17Z

LGTM

jessfraz · 2015-02-20T00:28:08Z

ping @crosbymichael or @LK4D4 :)

LK4D4 · 2015-02-20T04:31:33Z

LGTM

fix tomlv because it doesn't read from stdin

jessfraz force-pushed the ugh-tomlv branch from 3252c2d to 88ff1f8 Compare February 18, 2015 08:00

GordonTheTurtle added the status/0-triage label Feb 18, 2015

jessfraz force-pushed the ugh-tomlv branch from 88ff1f8 to 3bc6935 Compare February 18, 2015 08:02

jessfraz added status/2-code-review and removed status/0-triage labels Feb 18, 2015

tianon reviewed Feb 18, 2015
View reviewed changes

jessfraz force-pushed the ugh-tomlv branch from 3bc6935 to 4987246 Compare February 18, 2015 22:00

jessfraz force-pushed the ugh-tomlv branch from 4987246 to c290b1f Compare February 18, 2015 22:05

jessfraz force-pushed the ugh-tomlv branch from c290b1f to 0ff516d Compare February 18, 2015 22:19

tianon reviewed Feb 18, 2015
View reviewed changes

jessfraz force-pushed the ugh-tomlv branch from 0ff516d to 17d6fc6 Compare February 19, 2015 00:12

jessfraz force-pushed the ugh-tomlv branch 2 times, most recently from 6c6e21b to b7b1e93 Compare February 19, 2015 07:12

jessfraz force-pushed the ugh-tomlv branch from b7b1e93 to 4b9eb83 Compare February 19, 2015 07:19

jessfraz force-pushed the ugh-tomlv branch 2 times, most recently from d3857ab to c802a34 Compare February 19, 2015 07:21

tianon reviewed Feb 19, 2015
View reviewed changes

jessfraz force-pushed the ugh-tomlv branch from c802a34 to 489e65d Compare February 19, 2015 18:13

jessfraz force-pushed the ugh-tomlv branch from 489e65d to 868cc8e Compare February 19, 2015 19:42

thaJeztah reviewed Feb 19, 2015
View reviewed changes

jessfraz force-pushed the ugh-tomlv branch from 868cc8e to 06685a5 Compare February 19, 2015 21:15

fix tomlv because it doesn't read from stdin

06685a5

Docker-DCO-1.1-Signed-off-by: Jessica Frazelle <jess@docker.com> (github: jfrazelle) Docker-DCO-1.1-Signed-off-by: Jessie Frazelle <jess@docker.com> (github: jfrazelle)

LK4D4 added a commit that referenced this pull request Feb 20, 2015

Merge pull request #10872 from jfrazelle/ugh-tomlv

a78ce5c

fix tomlv because it doesn't read from stdin

LK4D4 merged commit a78ce5c into moby:master Feb 20, 2015

dmp42 removed the status/2-code-review label Feb 20, 2015

jessfraz deleted the ugh-tomlv branch February 20, 2015 05:13

SvenDowideit mentioned this pull request Feb 26, 2015

possible PRs to merge for a docs-update #11031

Closed

fix tomlv because it doesn't read from stdin #10872

fix tomlv because it doesn't read from stdin #10872

Conversation

jessfraz commented Feb 18, 2015

LK4D4 commented Feb 18, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tianon commented Feb 18, 2015

jessfraz commented Feb 18, 2015

jessfraz commented Feb 18, 2015

tianon commented Feb 18, 2015

tianon commented Feb 18, 2015

jessfraz commented Feb 18, 2015

tianon commented Feb 18, 2015

tianon commented Feb 18, 2015

jessfraz commented Feb 18, 2015

tianon commented Feb 18, 2015

tianon commented Feb 18, 2015

jessfraz commented Feb 18, 2015

tianon commented Feb 18, 2015

tianon commented Feb 18, 2015

Choose a reason for hiding this comment

jessfraz commented Feb 19, 2015

jessfraz commented Feb 19, 2015

jessfraz commented Feb 19, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jessfraz commented Feb 19, 2015

jessfraz commented Feb 19, 2015

thaJeztah commented Feb 19, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tianon commented Feb 20, 2015

jessfraz commented Feb 20, 2015

LK4D4 commented Feb 20, 2015