Issue #143 Check the DELETED event on Watch Builds #276

kishansagathiya · 2018-09-27T10:19:46Z

Currently we look at Object.Status.Phase to check the status of build.
This approach works, except for DELETED event. OpenShift would show the
previous phase on deleting, giving us a wrong impression of build
status.
Check for o.Type as well.

After this change, if we reset the environment, we won't see any active builds as well used to see earlier (OK Tested Locally)
but the previous build of the user will still be shown in the done build, which I think is appropriate, so no need to change that.

Fixed #143

alien-ike · 2018-09-27T10:19:47Z

Ike Plugins (test-keeper)

Thank you @kishansagathiya for this contribution!

It seems that this PR already contains some added or changed tests. Good job!

For more information please head over to official documentation. You can find there how to configure the plugin.

sthaha · 2018-09-27T12:50:28Z

internal/openshift/controller.go

@@ -89,7 +89,7 @@ func (c *controllerImpl) HandleBuild(o model.Object) error {
 	sendUserToIdler := false
 	userIdler := c.userIdlerForNamespace(ns)
 	user := userIdler.GetUser()
-	if c.isActive(&o.Object) {
+	if c.isActive(&o.Object) && o.Type != "DELETED" {


shoudn't we change isActive instead to return false if o.Type == "DELETED" ?

sthaha · 2018-09-27T12:51:01Z

internal/openshift/controller_test.go

@@ -52,6 +52,10 @@ func Test_handle_build(t *testing.T) {

 	err := controller.HandleBuild(obj)
 	assert.NoError(t, err)
+
+	obj.Type = "DELETED"


can you add a test for the case where the type isn't "DELETED" ?

There is already one test above that is not DELETED, but let me add tests for all WatchEvents https://github.com/kubernetes/kubernetes/blob/932e657d5d0e98624fb9907ea34a55ac604253d3/pkg/watch/json/types.go#L35

Added extra WatchEvent types. but whatever tests that we have don't seem to do much. Do you want me to add some better tests?
(sorry for rebase. I made a mistake while using git. Had to do it.)

Do you want me to add some better tests?

Yes, please. the current tests do not assert changes to controllers internals i.e. other than testing that it does not result in an error. Ideally you would want tests to invoke the method and assert that the internal state changes are as expected.

kishansagathiya · 2018-09-28T04:25:35Z

upstream issue filed for this openshift/origin#21112

sthaha · 2018-10-09T11:38:52Z

internal/util/strings.go

@@ -23,3 +25,8 @@ func Contains(list []string, s string) bool {
 	}
 	return false
 }
+
+// RandomString returns random string of length len
+func RandomString(len int) string {


I think you could use https://golang.org/src/math/rand/rand.go?s=11556:11594#L341

https://golang.org/pkg/math/rand/#Read

I don't see how that is better than what is proposed.

in general i would say it's better to use standard function from the go std package than rolling your own,

I will get rid of this function. But still using the same body. strconv.Itoa(rand.Intn(1000))(go std package) is still better than rand.Read(p []byte). Why? https://syslog.ravelin.com/byte-vs-string-in-go-d645b67ca7ff (you would mostly be aware of contents in that link, just keeping there for reference)

@chmouel May be I am guilty of premature optimization here 😜

c36a029#diff-6a1732973a3d77ccbc280c5f8e106be8R110

sthaha · 2018-10-09T11:41:20Z

internal/openshift/controller.go

-func (c *controllerImpl) isActive(b *model.Build) bool {
-	return model.Phases[b.Status.Phase] == 1
+func isActive(b model.Object) bool {
+	return model.Phases[b.Object.Status.Phase] == 1 && b.Type != "DELETED"


Do you know what different values can 'b.Type` hold?

https://github.com/kubernetes/apimachinery/blob/76721d167b7088b203cf9873c616b0cfec5c77bb/pkg/watch/watch.go#L44

2 things

there are other values as well. I guess error should ignored as well or rather only look at Added, Modfied

can we just the enum defined there instead of the "DELETED" magic word?

Yes, there are other types. You should be able to find this link and all the types used in the proposed change.
Yes, we can check for other types, but why? The function is sufficient to check what we need. Checking those types add no value, you would still need to check for the phase.

sthaha · 2018-10-09T11:50:35Z

internal/openshift/controller_test.go

+	testWithStateChange(t, obj)
+}
+
+func testWithStateChange(t *testing.T, obj model.Object) {


Good stuff!

chmouel · 2018-09-27T11:17:41Z

internal/openshift/controller.go

@@ -89,7 +89,7 @@ func (c *controllerImpl) HandleBuild(o model.Object) error {
 	sendUserToIdler := false
 	userIdler := c.userIdlerForNamespace(ns)
 	user := userIdler.GetUser()
-	if c.isActive(&o.Object) {
+	if c.isActive(&o.Object) && o.Type != "DELETED" {


this sound okay to me althoiugh it would be nice to have a comment explaining what you are saying in the commit message.

If that's the default behaviour of k8/openshfit can we have a bug report on openshift origin issues because I don't think it should behave like this,

chmouel · 2018-10-11T11:32:33Z

internal/openshift/controller.go

@@ -14,6 +14,7 @@ import (
 	"github.com/fabric8-services/fabric8-jenkins-idler/internal/tenant"
 	"github.com/fabric8-services/fabric8-jenkins-idler/internal/toggles"
 	"github.com/sirupsen/logrus"
+	"k8s.io/apimachinery/pkg/watch"


I believe that If you aAre you adding this you need to modify the gopkg.toml too, or regenerate the lock file

chmouel · 2018-10-11T11:33:20Z

internal/openshift/controller.go

@@ -138,7 +137,7 @@ func (c *controllerImpl) HandleBuild(o model.Object) error {
 	// So we need to clean up the Active build ref.
 	if user.ActiveBuild.Metadata.Name == user.DoneBuild.Metadata.Name {
 		log.Infof("Active and Done builds are the same (%s), cleaning active builds", user.ActiveBuild.Metadata.Name)
-		user.ActiveBuild = model.Build{Status: model.Status{Phase: "New"}}
+		user.ActiveBuild = model.Build{}


is it implied?

So, on finding the current build to be done, it would move it to done, but it would also mark a new active build with status New, without checking whether there actually is a new build or not.
That seemed wrong to me. It should instead use an empty Build object.

chmouel · 2018-10-11T11:34:39Z

internal/util/strings.go

@@ -23,3 +25,8 @@ func Contains(list []string, s string) bool {
 	}
 	return false
 }
+
+// RandomString returns random string of length len
+func RandomString(len int) string {


in general i would say it's better to use standard function from the go std package than rolling your own,

kishansagathiya · 2018-10-12T05:33:15Z

Gopkg.lock

+  packages = ["."]
+  revision = "298182f68c66c05229eb03ac171abe6e309ee79a"
+  version = "v1.0.3"
+


@chmouel This gets generated with dep ensure. It not being used in idler, but it is one of the dependencies in k8s.io/apimachinery. So, I think that's why it is here.

kishansagathiya · 2018-10-12T08:44:01Z

@chmouel @sthaha Rebased against master

Currently we look at Object.Status.Phase to check the status of build. This approach works, except for DELETED event. OpenShift would show the previous phase on deleting, giving us a wrong impression of build status. Check for o.Type as well. Added tests for HandleBuilds No such phase as `Finished`, add `Error` phase Some method could just be functions

Added dependencies and regerated lock file

kishansagathiya requested review from sthaha and chmouel September 27, 2018 10:20

sthaha reviewed Sep 27, 2018

View reviewed changes

sthaha suggested changes Sep 27, 2018

View reviewed changes

kishansagathiya force-pushed the issue_143 branch from c512989 to 2d75659 Compare September 28, 2018 05:44

sthaha reviewed Oct 9, 2018

View reviewed changes

kishansagathiya force-pushed the issue_143 branch from 721ba9f to be1e757 Compare October 11, 2018 06:28

chmouel reviewed Oct 11, 2018

View reviewed changes

kishansagathiya commented Oct 12, 2018

View reviewed changes

chmouel approved these changes Oct 12, 2018

View reviewed changes

kishansagathiya force-pushed the issue_143 branch from c36a029 to 6d86b9a Compare October 12, 2018 08:42

kishansagathiya added 2 commits October 15, 2018 12:21

Issue #143 Check the DELETED event on Watch Builds

21d20cd

Added dependencies and regerated lock file

kishansagathiya force-pushed the issue_143 branch from 6d86b9a to 21d20cd Compare October 15, 2018 06:51

kishansagathiya assigned sthaha Nov 19, 2018

hrishin added this to the 159/L milestone Dec 21, 2018

hrishin modified the milestones: 159/L, 160/L Jan 7, 2019

kishansagathiya closed this by deleting the head repository Dec 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #143 Check the DELETED event on Watch Builds #276

Issue #143 Check the DELETED event on Watch Builds #276

kishansagathiya commented Sep 27, 2018

alien-ike commented Sep 27, 2018 •

edited

Loading

sthaha Sep 27, 2018

sthaha Sep 27, 2018

kishansagathiya Sep 28, 2018

kishansagathiya Sep 28, 2018

sthaha Sep 28, 2018

kishansagathiya commented Sep 28, 2018

sthaha Oct 9, 2018

sthaha Oct 9, 2018

kishansagathiya Oct 10, 2018

chmouel Oct 11, 2018

kishansagathiya Oct 12, 2018 •

edited

Loading

kishansagathiya Oct 12, 2018

sthaha Oct 9, 2018

sthaha Oct 9, 2018

kishansagathiya Oct 10, 2018 •

edited

Loading

sthaha Oct 9, 2018

chmouel Sep 27, 2018

chmouel Oct 11, 2018

chmouel Oct 11, 2018

kishansagathiya Oct 12, 2018

chmouel Oct 11, 2018

kishansagathiya Oct 12, 2018

kishansagathiya commented Oct 12, 2018

Issue #143 Check the DELETED event on Watch Builds #276

Issue #143 Check the DELETED event on Watch Builds #276

Conversation

kishansagathiya commented Sep 27, 2018

alien-ike commented Sep 27, 2018 • edited Loading

Ike Plugins (test-keeper)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kishansagathiya commented Sep 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kishansagathiya Oct 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kishansagathiya Oct 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kishansagathiya commented Oct 12, 2018

alien-ike commented Sep 27, 2018 •

edited

Loading

kishansagathiya Oct 12, 2018 •

edited

Loading

kishansagathiya Oct 10, 2018 •

edited

Loading