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

Separate cmd from inspector impl #7

Merged
merged 1 commit into from
Apr 11, 2016
Merged

Conversation

pweil-
Copy link
Contributor

@pweil- pweil- commented Apr 6, 2016

Builds on #6 removed

Second commit separates the command from the inspector implementation

[pweil@localhost image-inspector]$ make clean verify test-unit
rm -rf _output Godeps/_workspace/pkg
hack/build-go.sh
hack/build-go.sh took 2 seconds
hack/verify-gofmt.sh
GOTEST_FLAGS="" hack/test-go.sh  
=== RUN   TestValidate
--- PASS: TestValidate (0.00s)
PASS
coverage: 75.0% of statements
ok      github.com/simon3z/image-inspector/pkg/cmd  1.005s
[DEBUG] Exit trap handler got return code 0
hack/test-go.sh succeeded after 4 seconds
[pweil@localhost image-inspector]$ _output/local/bin/local/image-inspector --image=fedora:22 --serve 0.0.0.0:8080
2016/04/06 13:25:59 Image fedora:22 is available, skipping image pull
2016/04/06 13:26:00 Extracting image fedora:22 to /var/tmp/image-inspector-275300167
2016/04/06 13:26:02 !!!WARNING!!! It is insecure to serve the image content without changing
2016/04/06 13:26:02 root (--chroot). Absolute-path symlinks in the image can lead to disclose
2016/04/06 13:26:02 information of the hosting system.
2016/04/06 13:26:02 Serving image content /var/tmp/image-inspector-275300167 on webdav://0.0.0.0:8080/api/v1/content/

@pweil- pweil- changed the title Separate cmd Separate cmd from inspector impl Apr 6, 2016
@simon3z
Copy link
Owner

simon3z commented Apr 7, 2016

@pweil- is it hard to decouple this from #6? I would like to merge this ASAP, and give more thought to #6.

@pweil-
Copy link
Contributor Author

pweil- commented Apr 7, 2016

nope, not hard. I can remove the scripts commit.

@simon3z
Copy link
Owner

simon3z commented Apr 7, 2016

@pweil- can you do that? I trust you ran the tests locally. Thanks.

@pweil-
Copy link
Contributor Author

pweil- commented Apr 7, 2016

yep, will do it today. Output of test is in the description (you can see between PRs it jumps from 6% coverage to 75% coverage due to separation of the files). Also looking at the easiest way to test some of the other pieces.

@enoodle
Copy link
Contributor

enoodle commented Apr 7, 2016

LGTM 👍 Thanks @pweil- ! This is great!

@pweil-
Copy link
Contributor Author

pweil- commented Apr 7, 2016

I wasn't able to get to this today due to blocker bugs. Will have to get it tomorrow. Sorry about that.

@pweil-
Copy link
Contributor Author

pweil- commented Apr 7, 2016

Actually I got to it... PTAL.

Here is how I'm building/testing without the scripts in #6. If there is anything else you'd like me to test before submitting a PR please let me know. I probably won't be able to get back to the other unit tests until at least Monday.

# My directory setup
[pweil@localhost image-inspector]$ pwd
/home/pweil/codebase/image-inspector/src/github.com/simon3z/image-inspector

# My GOPATH
[pweil@localhost image-inspector]$ echo $GOPATH
/home/pweil/codebase/image-inspector/src/github.com/simon3z/image-inspector/Godeps/_workspace/:/home/pweil/codebase/image-inspector

# Building the command
[pweil@localhost image-inspector]$ go build cmd/image-inspector.go 

# Test run of the command
[pweil@localhost image-inspector]$ ./image-inspector --image=fedora:22 --serve 0.0.0.0:8080
2016/04/07 19:49:12 Image fedora:22 is available, skipping image pull
2016/04/07 19:49:12 Extracting image fedora:22 to /var/tmp/image-inspector-462332336
2016/04/07 19:49:15 !!!WARNING!!! It is insecure to serve the image content without changing
2016/04/07 19:49:15 root (--chroot). Absolute-path symlinks in the image can lead to disclose
2016/04/07 19:49:15 information of the hosting system.
2016/04/07 19:49:15 Serving image content /var/tmp/image-inspector-462332336 on webdav://0.0.0.0:8080/api/v1/content/

# Run tests with coverage
[pweil@localhost image-inspector]$ go test -v -cover ./...
?       github.com/simon3z/image-inspector/cmd  [no test files]
=== RUN   TestValidate
--- PASS: TestValidate (0.00s)
PASS
coverage: 75.0% of statements
ok      github.com/simon3z/image-inspector/pkg/cmd  0.001s  coverage: 75.0% of statements
?       github.com/simon3z/image-inspector/pkg/inspector    [no test files]

if _, err := client.InspectImage(i.opts.Image); err != nil {
log.Printf("Pulling image %s", i.opts.Image)
imagePullOption := docker.PullImageOptions{Repository: i.opts.Image}
imagePullAuths, authCfgErr := getAuthConfigs(i.opts.DockerCfg, i.opts.Username, i.opts.PasswordFile)
Copy link
Owner

Choose a reason for hiding this comment

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

@pweil- can this be simplified with:

if imagePullAuths, err := getAuthConfigs(i.opts.DockerCfg, i.opts.Username, i.opts.PasswordFile); err != nil {
    return err
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we could but we'd have to pre-declare the imagePullAuths so I figured it's same/same either way (new error or new line for imagePullAuths). Happy to update it though.

For the record I didn't reuse err so we wouldn't shadow it in case it was ever used later in the life of this code.

@simon3z
Copy link
Owner

simon3z commented Apr 8, 2016

@pweil- I had a couple of comments, the rest LGTM.

@pweil-
Copy link
Contributor Author

pweil- commented Apr 8, 2016

@simon3z - fixed for your comments are in commit 2. If it looks good I'll squash.

@simon3z
Copy link
Owner

simon3z commented Apr 8, 2016

LGTM 👍 thanks @pweil- (I'll go over it once again to check everything)

@pweil-
Copy link
Contributor Author

pweil- commented Apr 8, 2016

squashed

@simon3z simon3z merged commit e24e4ce into simon3z:master Apr 11, 2016
enoodle pushed a commit to enoodle/image-inspector that referenced this pull request May 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants