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 the ability to run dgoss when the docker daemon is not local. #271

Merged
merged 2 commits into from
Aug 18, 2017

Conversation

plispe
Copy link
Contributor

@plispe plispe commented Aug 7, 2017

I created pull request which is discussed in #269 and #243. All the changes should be backwards compatible.

@aelsabbahy
Copy link
Member

I'm guessing the #243 link was a mistake.

Skimmed through the code and I like this approach. I'll test it and provide feedback this week.

@plispe
Copy link
Contributor Author

plispe commented Aug 7, 2017

Yes, #243 link was a mistake. Sorry I meant #246.

info "Starting docker container"
docker start $id > /dev/null
;;
*) error "Wrong goss files strategy used! Correct options are \"mount\" of \"cp\"."
Copy link
Contributor

Choose a reason for hiding this comment

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

mount or copy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@elliotweiser Ok I will change cp to copy. I used cp only because the docker cp subcommand.

Copy link
Member

Choose a reason for hiding this comment

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

Cp is fine, I think it's the "of" instead of "or" that needs fixing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes. "of' is a typo. sorry for that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep just a typo: I meant of vs or.

Whatever your preference is for copy vs cp ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry about that... my brain did an auto-complete 😅

@elliotweiser
Copy link
Contributor

It should be possible to copy the goss.yaml file out of the running docker container when using the copy option, no?

@plispe
Copy link
Contributor Author

plispe commented Aug 7, 2017

@elliotweiser I think, that goss.yaml is copied only when you use dgoss edit not when dgoss run is used. The dgoss edit should work as expected.

@elliotweiser
Copy link
Contributor

elliotweiser commented Aug 7, 2017

You are correct about the goss.yaml not being copied out of the container when using dgoss run.

I guess I'm responding to this statement:

With the 'cp' strategy you lose the ability to write tests or waits against the docker output.

Since the generated goss.yaml would normally live on the volume, retrieving it after dgoss edit is trivial. When using the cp mode, I think the original behavior can be preserved by executing a docker cp <CONTAINER>:<file> <file>.

(I personally can't think of a good reason for developing tests against a remote docker daemon, but for the sake of thorough code review, I thought I'd point this out).

Overall, LGTM 👍

@plispe
Copy link
Contributor Author

plispe commented Aug 7, 2017

Since the generated goss.yaml would normally live on the volume, retrieving it after dgoss edit is trivial. When using the cp mode, I think the original behaviour can be preserved by executing a docker cp :file file.

There is actually no volume used with dgoss edit but, the generated goss.yaml is copied with docker cp command from the container. Therefore dgoss edit is unaffected by my changes.

With the 'cp' strategy you lose the ability to write tests or waits against the docker output.

This statement is about the following command from dgoss script docker logs -f "$id" > "$tmp_dir/docker_output.log" 2>&1 & whose behaviour can not be preserved with cp strategy. Because you have no possibility to stream docker log -f output to the running container without a mounted volume.

@elliotweiser
Copy link
Contributor

I stand corrected: dgoss edit indeed doesn't use docker volumes. In which case...

:shipit: !

@plispe
Copy link
Contributor Author

plispe commented Aug 7, 2017

@elliotweiser

(I personally can't think of a good reason for developing tests against a remote docker daemon, but for the sake of thorough code review, I thought I'd point this out).

The only use-case could be the use of CicleCI Debugging Jobs Over SSH as CircleCI runs jobs in separated environments. I debug sometimes my failed builds over ssh.

The reason for this PR was actually the fact, that dgoss does not run with CircleCI at this time.

@elliotweiser
Copy link
Contributor

Neat! I'll have to check that out. Thanks for the contribution and the thorough responses!

@plispe
Copy link
Contributor Author

plispe commented Aug 7, 2017

You are welcome.

@aelsabbahy
Copy link
Member

Apologies for the delay on this, looks great. Thanks for the contribution!

@plispe
Copy link
Contributor Author

plispe commented Aug 18, 2017

No problem. I used my fork untill now. Thanks for merge.

@plispe plispe mentioned this pull request Jan 10, 2018
BenjaminHerbert pushed a commit to BenjaminHerbert/goss that referenced this pull request May 28, 2020
…ss-org#271)

* Add the ability to run dgoss when the docker daemon is not local

* Fix a typo
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.

None yet

3 participants