-
-
Notifications
You must be signed in to change notification settings - Fork 17
Add network->enable feature to compose file #47
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 network->enable feature to compose file #47
Conversation
Signed-off-by: Paulo Almeida <paulo.almeidarodenas@niwa.co.nz>
Signed-off-by: Paulo Almeida <paulo.almeidarodenas@niwa.co.nz>
Signed-off-by: Paulo Almeida <paulo.almeidarodenas@niwa.co.nz>
scompose/project/instance.py
Outdated
|
|
||
| # if not specified, set the default value for enable property | ||
| if "enable" not in self.network: | ||
| self.network["enable"] = True |
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.
For these two, how about:
for key in ["enable", "allocate_ip"]:
self.network[key] = self.network.get(key, True)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.
I like the suggestion! Will do it it now
vsoch
left a comment
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.
Oh I absolutely love this!! I made a suggestion for a more concise population of True values, and other than that let's just add a note to the CHANGELOG and another tiny version bump.
Signed-off-by: Paulo Almeida <paulo.almeidarodenas@niwa.co.nz>
|
Really liked the suggestion, thanks for that @vsoch I just implemented the suggestions that you gave, let me know if there is anything else that I can do on this PR |
| options += self._get_network_commands(ip_address) | ||
|
|
||
| # Hostname | ||
| options += ["--hostname", self.name] |
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.
I haven't tried this so I don't know the answer - do we want a hostname if we don't enable network options?
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.
Good point. I tried that. It doesn't enable the network options.. I agree that sounds counter intuitive but singularity is funny sometimes, ay? 😂
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.
But if you want I can move the --hostname under the same if statement.
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.
I think it's okay to leave - if there is some addition to a resolver file that gives the container a hostname without setting up networking, I suppose someone could want that? And it doesn't hurt to have it. So let's leave as is until there is more evidence to change that.
By default singularity-compose will always append --net to command to be executed in which it will prompt for either having --network=none or --fakeroot added.
Depending on your environment's configuration and isolation requirements you may want to be able to instruct singularity-compose not to append --net or any network-related params to the command sent to singularity CLI.
The example below will disable the network features: