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

Multiple dockercfg #9

Merged
merged 1 commit into from
Apr 28, 2016
Merged

Multiple dockercfg #9

merged 1 commit into from
Apr 28, 2016

Conversation

enoodle
Copy link
Contributor

@enoodle enoodle commented Apr 10, 2016 •
edited
Loading

Adds support for multiple dockercfg files defined by the user with repeated flags: --dockercfg file1 --dockercfg file2. This will try to pull the image with all the authentications from all the files. (Assuming the user is not particular with which authentication he wants to use otherwise he would have only sent that one..):

sudo ./image-inspector --image=registry.access.redhat.com/openshift3/metrics-cassandra --chroot --serve=0.0.0.0:5000 --path=/tmp/image-content/ --dockercfg dockercfg1 --dockercfg dockercfg2

@enoodle
Copy link
Contributor Author

enoodle commented Apr 10, 2016

This is rebased on the split patch from @pweil- , I will rebase on master once it its merged. @pweil- I will appreciate a review, especially on the test file.

@enoodle enoodle force-pushed the multiple_dockercfg branch 2 times, most recently from 7629f71 to f328861 Compare April 11, 2016 07:49
simon3z
simon3z reviewed Apr 11, 2016
View reviewed changes
pkg/inspector/image-inspector.go
@@ -93,7 +93,7 @@ func (i *defaultImageInspector) Inspect() error {
container, err := client.CreateContainer(docker.CreateContainerOptions{
Name: randomName,
Config: &docker.Config{
Image: i.opts.Image,
Image: i.opts.Image,
Copy link
Owner

Choose a reason for hiding this comment

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

@enoodle why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am sorry, Its a problem with my vim and its lint.. its autocorrects this. I will look into this.. Oo

@enoodle enoodle force-pushed the multiple_dockercfg branch from f328861 to 5e43312 Compare April 13, 2016 13:47
@enoodle
Copy link
Contributor Author

enoodle commented Apr 13, 2016

I rebased it on #3 just for convenience with future merge. Will rebase on master once #3 is merged.

@enoodle enoodle force-pushed the multiple_dockercfg branch 8 times, most recently from 6fb3d2b to 23b617a Compare April 18, 2016 14:18
simon3z
simon3z reviewed Apr 18, 2016
View reviewed changes
pkg/inspector/image-inspector_test.go
}

func TestGetAuthConfigs(t *testing.T) {

Copy link
Owner

Choose a reason for hiding this comment

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

@enoodle extra while line

@enoodle enoodle force-pushed the multiple_dockercfg branch from 23b617a to da2d0a2 Compare April 20, 2016 13:26
@simon3z
Copy link
Owner

simon3z commented Apr 20, 2016

@pweil- @enoodle this is next. Please re-test / re-review and let's get this in as well.

@enoodle enoodle force-pushed the multiple_dockercfg branch from da2d0a2 to 8e8cbc7 Compare April 21, 2016 11:43
@enoodle
Copy link
Contributor Author

enoodle commented Apr 21, 2016 •
edited
Loading

=== RUN TestValidate
--- PASS: TestValidate (0.00s)
PASS
coverage: 100.0% of statements
ok github.com/simon3z/image-inspector/pkg/cmd 0.011s coverage: 100.0% of statements
=== RUN TestScanImage
2016/04/21 16:20:43 MockScanner scanning here. Placing results in
2016/04/21 16:20:43 MockScanner scanning here. Placing results in
2016/04/21 16:20:43 MockScanner scanning here. Placing results in
--- PASS: TestScanImage (0.00s)
=== RUN TestGetAuthConfigs
--- PASS: TestGetAuthConfigs (0.00s)
=== RUN TestCreateOutputDir
--- PASS: TestCreateOutputDir (0.00s)
PASS
coverage: 22.5% of statements
ok github.com/simon3z/image-inspector/pkg/inspector 0.005s coverage: 22.5% of statements
=== RUN TestGetRhelDist
--- PASS: TestGetRhelDist (0.00s)
=== RUN TestScan
--- PASS: TestScan (0.00s)
PASS
coverage: 38.3% of statements
ok github.com/simon3z/image-inspector/pkg/openscap 0.025s coverage: 38.3% of statements

pweil-
pweil- reviewed Apr 21, 2016
View reviewed changes
pkg/inspector/image-inspector_test.go
shouldFail bool
}{
"two dockercfg": {opts: goodTwoDockerCfg, shouldFail: false},
"username and passwordFile": {opts: goodUserAndPass, shouldFail: false},
Copy link

Choose a reason for hiding this comment

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

should we add failing tests too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

@simon3z
Copy link
Owner

simon3z commented Apr 21, 2016

@enoodle for the record can you drop here in the PR comments how the command line looks like? Thanks.

@enoodle enoodle force-pushed the multiple_dockercfg branch from 8e8cbc7 to 8c21e1c Compare April 21, 2016 13:20
@enoodle
Copy link
Contributor Author

enoodle commented Apr 21, 2016

@simon3z added example command line in the PR comment

@enoodle enoodle force-pushed the multiple_dockercfg branch from 8c21e1c to ac92a0f Compare April 21, 2016 15:10
@simon3z
Copy link
Owner

simon3z commented Apr 22, 2016

@pweil- LGTY?

@pweil-
Copy link

pweil- commented Apr 22, 2016

All my comments have been addressed. LGTM. Thanks @enoodle!

@enoodle
Copy link
Contributor Author

enoodle commented Apr 27, 2016

@simon3z ping

@enoodle enoodle force-pushed the multiple_dockercfg branch from ac92a0f to 8d10a87 Compare April 28, 2016 09:40
@simon3z simon3z merged commit 2a1c4ea into simon3z:master Apr 28, 2016
@enoodle enoodle mentioned this pull request May 10, 2016
Closed
enoodle pushed a commit to enoodle/image-inspector that referenced this pull request May 18, 2016
@simon3z
Merge pull request simon3z#9 from pweil-/e2e
363de44 
e2e tests initial commit
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@enoodle @simon3z @pweil-