-
Notifications
You must be signed in to change notification settings - Fork 27
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
Resolve network configuration issues #115
Resolve network configuration issues #115
Conversation
Signed-off-by: Atanas Dinov <[email protected]>
Signed-off-by: Atanas Dinov <[email protected]>
Signed-off-by: Atanas Dinov <[email protected]>
@@ -19,10 +19,10 @@ const ( | |||
// Used for both input component source and | |||
// output configurations subdirectory under combustion. | |||
networkConfigDir = "network" | |||
networkConfigScriptName = "configure-network.sh" | |||
networkConfigScriptName = "03-configure-network.sh" |
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.
This will now always be the first script we execute. I intentionally didn't go for 01 or 02 in case we'd want to plug in something more important before this but I'm open to changing it as well.
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 to leave 01/02 free... 03 is good as we really need to think about bringing up the network really early in the process given the other network-related items that are configured.
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.
Agreed that we want to leave some way to inject things at the very start. I still think we need a more comprehensive numbering review once we get close to MVP, though I don't see this one moving from the start.
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.
LGTM. Have tested successfully 🚀 thanks!
set -euo pipefail | ||
|
||
{{ if .NetworkScript -}} |
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.
Surprised we made it this long without needing to templatize the base combustion script.
exit 0 | ||
fi | ||
{{- else -}} | ||
# combustion: network |
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.
This is so funny to me since it looks like an else block that simply lays down a comment, but the implications are enormous.
Closes: