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

FreeBSD support using rsync mechanism #458

Merged
merged 1 commit into from Sep 11, 2017

Conversation

Projects
None yet
3 participants
@omab
Contributor

omab commented Sep 4, 2017

This PR brings support for FreeBSD platform using rsync mechanism (unison should also be possible but I haven't tried it yet).

Docker support on FreeBSD is possible through docker-machine using virtualbox running a boot2docker virtualmachine which is then connected to the host by the generic driver.

Overall changes:

  • Support for FreeBSD environment
  • Configurable rsyncd allowed hosts by sync_host_allow docker-sync.yml value

@omab omab force-pushed the omab:master branch from 336bcbe to 872c069 Sep 4, 2017

@EugenMayer

This comment has been minimized.

Owner

EugenMayer commented Sep 5, 2017

thats very interesting, thank you for putting work into that! i have to review that for a bit, but i am sure to be interested to merge it.

@omab

This comment has been minimized.

Contributor

omab commented Sep 5, 2017

@EugenMayer, thanks, let me know if any change is required.

@EugenMayer

This comment has been minimized.

Owner

EugenMayer commented Sep 6, 2017

@omab would be great if you could fix / comment on the above

@omab

This comment has been minimized.

Contributor

omab commented Sep 6, 2017

@EugenMayer I don't see any comment

Docker.ensure!
Unison.ensure! if config.unison_required?
Rsync.ensure! if config.rsync_required?
Fswatch.ensure! if config.fswatch_required?

This comment has been minimized.

@EugenMayer

EugenMayer Sep 6, 2017

Owner

fswatch should be OSX related, has nothing to do here

This comment has been minimized.

@omab

omab Sep 7, 2017

Contributor

Removed.

@@ -97,7 +97,7 @@ def start_container
user_mapping = get_user_mapping
group_mapping = get_group_mapping
cmd = "docker run -p '#{@options['sync_host_port']}:873' -v #{volume_name}:#{@options['dest']} #{user_mapping} #{group_mapping} -e VOLUME=#{@options['dest']} -e TZ=${TZ-`readlink /etc/localtime | sed -e 's,/usr/share/zoneinfo/,,'`} --name #{container_name} -d #{@docker_image}"
cmd = "docker run -p '#{@options['sync_host_port']}:873' -v #{volume_name}:#{@options['dest']} #{user_mapping} #{group_mapping} -e VOLUME=#{@options['dest']} -e TZ=${TZ-`readlink /etc/localtime | sed -e 's,/usr/share/zoneinfo/,,'`} -e ALLOW=#{@options['sync_host_allow']} --name #{container_name} -d #{@docker_image}"

This comment has been minimized.

@EugenMayer

EugenMayer Sep 6, 2017

Owner

that looks a bit unplanned, you changed somehting here which has not been part of the intention of the PR, right?

This comment has been minimized.

@omab

omab Sep 7, 2017

Contributor

The ALLOW rule is to let it override the value here entrypoint.sh#L86, I came across this need because Virtualbox will use IPs in the 10.0.0.0/8 net when using the host-only network interface.

Some options to improve it:

  1. Revert the change in this file and propose a PR against https://github.com/EugenMayer/docker-image-rsyncd to improve the IP range to support the 10.0.0.0/8 private network
  2. Keep the override but let it be included only if the option was defined

This comment has been minimized.

@EugenMayer

EugenMayer Sep 11, 2017

Owner

well i see what you mean, but then add the docs for this option sync_host_allow
here https://github.com/EugenMayer/docker-sync/blob/master/example/docker-sync.yml
and here https://github.com/EugenMayer/docker-sync/wiki/2.-Configuration
so people know about it, be sure to add a comment what this is for.

thanks

This comment has been minimized.

@ignatiusreza

ignatiusreza Sep 11, 2017

Collaborator

Don't we need default value for it? What if it's missing from the config?

This comment has been minimized.

@EugenMayer

EugenMayer Sep 11, 2017

Owner

we cannot "guess" what private network there might be - so there cannot be a default value, right?

This comment has been minimized.

@ignatiusreza

ignatiusreza Sep 11, 2017

Collaborator

but, without any default, -e ALLOW=#{@options['sync_host_allow']} will turn into -e ALLOW= which I assume is not a valid syntax?

if we can't have default, then the argument should be added only when @options['sync_host_allow'] is not blank?

This comment has been minimized.

@EugenMayer

EugenMayer Sep 11, 2017

Owner

sure that will be needed, right - so if blanc, skip the whole argument

This comment has been minimized.

@omab

omab Sep 11, 2017

Contributor

If value is blank, then bash will default to 192.168.0.0/16 172.16.0.0/12 since the rule is constructed like: ALLOW=${ALLOW:-192.168.0.0/16 172.16.0.0/12}, still I do agree that's cleaner to just avoid the parameter if unset.

This comment has been minimized.

@omab

omab Sep 11, 2017

Contributor

@EugenMayer, @ignatiusreza, changes done

@EugenMayer

This comment has been minimized.

Owner

EugenMayer commented Sep 6, 2017

sorry, forgot to commit the review, my bad

@omab omab force-pushed the omab:master branch from 872c069 to 0c456d5 Sep 7, 2017

@omab omab force-pushed the omab:master branch from 0c456d5 to c60b82b Sep 11, 2017

@EugenMayer

This comment has been minimized.

Owner

EugenMayer commented Sep 11, 2017

that looks very solid now, wonderfull. Now lets finish the docs.

I added freebsd in the TOC: https://github.com/EugenMayer/docker-sync/wiki and added a page here:
https://github.com/EugenMayer/docker-sync/wiki/docker-sync-on-freebsd - please write some basic docs for how to get things running under freebsd on what type of syncs are supported, e.g. like for linux and windows ( yours is more a like linux )

When we have all this stuff in place, i am very happy to add freeBSD here, very nice!

@EugenMayer EugenMayer added this to the 0.5.0 milestone Sep 11, 2017

@omab

This comment has been minimized.

Contributor

omab commented Sep 11, 2017

@EugenMayer, I've updated the wiki page as requested. Thanks!

@EugenMayer

This comment has been minimized.

Owner

EugenMayer commented Sep 11, 2017

great, thank you for your work. I added a note about native_osx - i understand its way to obvious for you not to mention it but i learned .. there is no such thing :)

Thank you for the patience and your effort in the docs, not only code wise!

@EugenMayer EugenMayer merged commit d1974b6 into EugenMayer:master Sep 11, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@omab

This comment has been minimized.

Contributor

omab commented Sep 11, 2017

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment