Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Testing build fix #2406

Merged
merged 3 commits into from
Jan 31, 2020
Merged

Testing build fix #2406

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
377ca01
Testing build fix
mik-dass Nov 21, 2019
d500359
Adds build timeout constant and some comments
mik-dass Dec 11, 2019
2fdd200
Adds comments for WaitForBuildToFinish()
mik-dass Dec 12, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions pkg/component/component.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -791,11 +791,8 @@ func Build(client *occlient.Client, componentName string, applicationName string
}

defer s.End(false)
if err := client.FollowBuildLog(buildName, stdout); err != nil {
return errors.Wrapf(err, "unable to follow logs for %s", buildName)
}

if err := client.WaitForBuildToFinish(buildName); err != nil {
if err := client.WaitForBuildToFinish(buildName, stdout); err != nil {
return errors.Wrapf(err, "unable to build %s, error: %s", buildName, b.String())
}
s.End(true)
Expand Down
43 changes: 29 additions & 14 deletions pkg/occlient/occlient.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1672,7 +1672,8 @@ func (c *Client) StartBuild(name string) (string, error) {
}

// WaitForBuildToFinish block and waits for build to finish. Returns error if build failed or was canceled.
func (c *Client) WaitForBuildToFinish(buildName string) error {
func (c *Client) WaitForBuildToFinish(buildName string, stdout io.Writer) error {
following := false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need more description on this, bit confused why we're setting following to false then true when going through the channel loop.

glog.V(4).Infof("Waiting for %s build to finish", buildName)

w, err := c.buildClient.Builds(c.Namespace).Watch(metav1.ListOptions{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand this is not touched in the proposed PR but can we add a comment that describes what this all about? At least I didn't understand a thing here. 😞

Expand All @@ -1682,23 +1683,36 @@ func (c *Client) WaitForBuildToFinish(buildName string) error {
return errors.Wrapf(err, "unable to watch build")
}
defer w.Stop()
timeout := time.After(5 * time.Minute)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be a constant? (high above in occlient.go)

for {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't understand what's going on in this for loop either. 😞

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can help here a bit. So here first thing to observe is the select statement on a quick TLDR: select statements are like channel collectors - listen on multiple channels at once, more here https://tour.golang.org/concurrency/5.

now the Build().Watch() returns a channel which returns messages on as per the pod status. Now the change that has been done here is
we wait for the pod to start before we start following the logs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as for the need for the following, The w.ResultChan() can send multiple messages of type BuildPhaseRunning. But we dont have to follow logs multiple times, just the first time should be good enough. Hence this flag.
Think of it as a singleton

val, ok := <-w.ResultChan()
if !ok {
break
}
if e, ok := val.Object.(*buildv1.Build); ok {
glog.V(4).Infof("Status of %s build is %s", e.Name, e.Status.Phase)
switch e.Status.Phase {
case buildv1.BuildPhaseComplete:
glog.V(4).Infof("Build %s completed.", e.Name)
return nil
case buildv1.BuildPhaseFailed, buildv1.BuildPhaseCancelled, buildv1.BuildPhaseError:
return errors.Errorf("build %s status %s", e.Name, e.Status.Phase)
select {
case val, ok := <-w.ResultChan():
if !ok {
break
}
if e, ok := val.Object.(*buildv1.Build); ok {
glog.V(4).Infof("Status of %s build is %s", e.Name, e.Status.Phase)
switch e.Status.Phase {
case buildv1.BuildPhaseComplete:
glog.V(4).Infof("Build %s completed.", e.Name)
return nil
case buildv1.BuildPhaseFailed, buildv1.BuildPhaseCancelled, buildv1.BuildPhaseError:
return errors.Errorf("build %s status %s", e.Name, e.Status.Phase)
case buildv1.BuildPhaseRunning:
// since the pod is ready and the build is now running, start following the logs
if !following {
following = true
err := c.FollowBuildLog(buildName, stdout)
if err != nil {
return err
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of this above looks good!

}
}
case <-timeout:
return errors.Errorf("timeout waiting for build %s to start", buildName)
}
}
return nil
}

// WaitAndGetDC block and waits until the DeploymentConfig has updated it's annotation
Expand Down Expand Up @@ -1845,6 +1859,7 @@ func (c *Client) FollowBuildLog(buildName string, stdout io.Writer) error {
}

rd, err := c.buildClient.RESTClient().Get().
Timeout(5*time.Minute).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be a constant

Namespace(c.Namespace).
Resource("builds").
Name(buildName).
Expand Down
2 changes: 1 addition & 1 deletion pkg/occlient/occlient_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2091,7 +2091,7 @@ func TestWaitForBuildToFinish(t *testing.T) {
return true, fkWatch, nil
})

err := fkclient.WaitForBuildToFinish(tt.buildName)
err := fkclient.WaitForBuildToFinish(tt.buildName, os.Stdout)
if !tt.wantErr == (err != nil) {
t.Errorf(" client.WaitForBuildToFinish(string) unexpected error %v, wantErr %v", err, tt.wantErr)
return
Expand Down