-
Notifications
You must be signed in to change notification settings - Fork 11
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
fix compose up wait for state #553
Conversation
for _, serviceInfo := range serviceInfos { | ||
serviceList = append(serviceList, compose.NormalizeServiceName(serviceInfo.Service.Name)) | ||
} | ||
var errFailedToReachStartedState = errors.New("failed to reach STARTED state") |
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.
declare errors where they are returned
} | ||
|
||
serviceState[newStatus.Name] = newStatus.State | ||
|
||
if allInState(targetState, serviceState) { | ||
for _, sInfo := range serviceInfos { |
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 was unused, so I removed it
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.
Added back to commands.go in #562, as the value is used in printEndpoints
@@ -38,14 +37,6 @@ func Subscribe(ctx context.Context, client client.Client, services []string) (<- | |||
defer serverStream.Close() | |||
defer close(statusChan) | |||
for { | |||
// handle cancel from caller; TODO: do we need this? Receive() should return false on context done |
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.
see comment; wasn't needed
// handle cancel from caller; TODO: do we need this? Receive() should return false on context done | ||
select { | ||
case <-ctx.Done(): | ||
term.Debug("Context Done - exiting Subscribe goroutine") |
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.
avoid printing in go-routine since it will mess up other prints in main program
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 more of a failure of our logging package than requirement, but I agree we should keep where logging happens, and try to avoid spread logging into all layers of the dependencies.
@@ -56,7 +47,6 @@ func Subscribe(ctx context.Context, client client.Client, services []string) (<- | |||
} | |||
continue | |||
} | |||
term.Debug("Subscribe Stream closed", serverStream.Err()) |
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.
ditto; prints moved to caller
if subStatus.Name == "" && (servInfo != nil && servInfo.Service != nil) { | ||
subStatus.Name = servInfo.Service.Name | ||
subStatus.Status = servInfo.Status | ||
subStatus.State = convertServiceState(servInfo.Status) | ||
} | ||
|
||
statusChan <- subStatus | ||
term.Debugf("service %s with state ( %s ) and status: %s\n", subStatus.Name, subStatus.State.String(), subStatus.Status) |
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.
ditto; moved to caller
683e927
to
479a908
Compare
|
||
// blocking call to tail | ||
if err := cli.Tail(ctx, client, tailParams); err != nil { | ||
if errors.Is(context.Cause(ctx), errDeploymentFailed) { | ||
term.Warn("Deployment FAILED. Service(s) not running.") | ||
|
||
if !nonInteractive { |
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.
non-interactive prompt in main thread now, which fixes the issue @raphaeltm was having
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
Refactor the async code. Please compare with #518, I think this one's cleaner.
We should introduce a build failure state to also trigger SERVICE_FAILED so we'll get AI-debugger for build failures. This requires some code in Fabric (probably needs to handle kaniko STOPPED state)