-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Implement golang bootstrapper #5399
Conversation
return fmt.Errorf("Port format is not supported %s", server.Port) | ||
} | ||
port := split[0] | ||
addr := "localhost:" + port |
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.
Are you sure localhost
is always available?
Please specify what kind of events will be emitted during the sample bootstrapping |
ok |
agents/bootstrapper/pom.xml
Outdated
<version>5.13.0-SNAPSHOT</version> | ||
</parent> | ||
<artifactId>bootstrapper</artifactId> | ||
<name>Bootstrapper</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.
Could you add more info into the artifact and name fields? E.g. agent-bootstrapper, Agent :: Bootstrapper
// AddAll adds batch of installers to the installation sequence. | ||
func AddAll(installers []Installer) { | ||
for _, installer := range installers { | ||
Add(installer) |
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.
Consider append(installers, newInstallers...)
instead
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.
makes sense
// If there is no installer which provides server, this func exits after | ||
// all the installation are completed. | ||
// In both cases if any error occurs during installation, | ||
// start exists returning that error. |
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 suppose exits
instead of exists
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.
thank you
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/2847/ |
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/2852/ |
func (svi *serverInst) preCheck() error { | ||
for _, server := range svi.installer.Servers { | ||
if tryConn(server) == nil { | ||
return fmt.Errorf("server address 'localhost:%s' already in use", server.Port) |
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 suppose that already running server is not a problem for an agent start. For example, ubuntu_jdk8 image start SSHD as the main command of the container. So with this check SSH agent will fail on that image.
return fmt.Errorf("Timeout reached before installation of '%s' finished", svi.installer.ID) | ||
case <-diedC: | ||
checker.stop() | ||
return fmt.Errorf("Process of installation '%s' exited before server became available", svi.installer.ID) |
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 suppose in case SSHD is already running, installation script exits immediately, so this error will occur even if the server is available. WDYT?
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 thought that execution of scripts which install servers must not exit, but it turns out that
exitCode == 0 + servers availability should be considered as successful installation.
I will rework it, thank you!
@garagatyi fixed, please review changes |
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.
Looks good. Good job!
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/2861/ |
Added golang app which implements bootstrapper spec(bootstrapper - agents workflow).
Usage
Example
For the configuration file
bootstrapper output will be
During the installation corresponding events are emitted
What issues does this PR fix or reference?
resolves #4099
spec - #4096 (comment)
new installation approach - #3971
Please note that these changes are going to be merged into SPI branch.