-
Notifications
You must be signed in to change notification settings - Fork 5
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
can now pass in a ip address or "dhcp" #333
base: master
Are you sure you want to change the base?
Conversation
rambo/options.py
Outdated
Return address (str) | ||
''' | ||
if not address: | ||
address = '192.168.33.10' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might as well take this opportunity to change this to the ip address vagrantup.com uses: 192.168.50.4
rambo/app.py
Outdated
@@ -383,6 +383,7 @@ def up(ctx=None, **params): | |||
params['ram_size'], params['drive_size'] = options.size_option( | |||
params.get('ram_size'), params.get('drive_size')) # both ram and drive size | |||
params['sync_dir'] = options.sync_dir_option(params.get('sync_dir')) | |||
params['address'] = options.address_option(params.get('address')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you put this at the top of the list so it's sorted? For a lot of stuff I wouldn't care, but I expect this list to get pretty long and that keep it tidier.
Some parts of cli.py should be sorted too, but that can wait for when the conf file loader thing is refactored. Doesn't make sense right now.
@@ -4,6 +4,19 @@ | |||
from rambo.settings import SETTINGS, PROJECT_LOCATION, PROJECT_NAME | |||
from rambo.utils import get_env_var, set_env_var | |||
|
|||
def address_option(address=None): | |||
'''Validate address. If "dhcp", get an address in 172.28 network. If not supplied, set to default 192.168.33.10. Set as env var. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no validation happening, really. We should check if it's the string 'dhcp', or somehow looks like an ip address. That's probably a googlable thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like Vagrant can accept an ipv4 or ipv6 address with the same syntax, which is nice. https://www.vagrantup.com/docs/networking/private_network.html So we can check for it being either with the above link.
Also a changelog entry would be nice. |
36f3bd4
to
0013db7
Compare
(issue reference)
Fixes #326
Does this deserve / include a changlog entry?