Add status section for Gitopsconfig CR#163
Add status section for Gitopsconfig CR#163seanmalloy merged 9 commits intoKohlsTechnology:masterfrom safirh:status_section
Conversation
safirh
changed the title
safirh
changed the title
akavel
left a comment
akavel
left a comment
There was a problem hiding this comment.
I am adding the first part of my feedback on this PR. I don't yet fully understand all the code, so I will try to come back to it tomorrow and continue reviewing. Anyway, I would like to already have some initial comments and questions, so I'm sending them now. Thanks!
akavel
left a comment
akavel
left a comment
There was a problem hiding this comment.
I like your approach in this PR quite much; I would like to especially highlight those aspects of it that impressed me:
- using the
.Status().Update()function — this is very important and crucial, thank you! - using
time.Sleep()in a loop inwatchjob— initially I thought we want to use Watch, but after reading about it, I now see it may be problematic, so just usingtime.Sleepis a great intermediate solution to have a working feature in time; - I like that you created the helper method
updateStatusfor wrapping a commonly repeated pattern of code and thus reducing the repetitiveness; - making the logic of the
watchjobfunc work well was not easy, and as far as I understand it, I believe you achieved this tricky task of making it work correctly.
In this review, I also added some comments, suggestions and questions with regards to some additional aspects of the code.
akavel
left a comment
akavel
left a comment
There was a problem hiding this comment.
I like it very much how your improvements are going!
After some of the cleanups you did, I now noticed a few more things that I didn't see before, and added comments about them. I think those would be my final major questions about the PR, and once we resolve them, I expect it would look reasonable to me for approval.
Thanks!
akavel
left a comment
akavel
left a comment
There was a problem hiding this comment.
Added comments about the new feature of runningJobMap which was introduced by new commits after my previous review.
akavel
left a comment
akavel
left a comment
There was a problem hiding this comment.
If @seanmalloy wants to accept this, from what I was able to understand, I think this code does display status information about jobs.
| //If reconciler calls the same cronjob before the final update of status, we might have an error while | ||
| //updating the status. To avoid this error runningJobMap is used. |
There was a problem hiding this comment.
Thank you very much for adding those kinds of good comments, explaining the rationale info for why some things were done, which is often not easy or even impossible to otherwise find in code. Highly appreciated!
|
@safirh the Travic CI tests have failed twice. Please investigate why the tests are failing. |
In issue24_test.go, the PR #163 (adding Status to the CR) was failing with the following error: time="2019-12-04T16:31:42+01:00" level=info msg="Started local operator" Found pod gcr.io/google-samples/hello-app:1.0 "hello-world-helm-76bc545546-zmp7v" Found pod gcr.io/google-samples/hello-app:1.0 "hello-world-helm-76bc545546-zmp7v" Found pod gcr.io/google-samples/hello-app:1.0 "hello-world-hierarchy-6b5cc6c69d-bvckb" Found pod gcr.io/google-samples/hello-app:1.0 "hello-world-issue24-fcc6c46f5-mmnnp" Found pod gcr.io/google-samples/hello-app:1.0 "hello-world-issue24-fcc6c46f5-mmnnp" Found pod gcr.io/google-samples/hello-app:2.0 "now-only-599f5bf5bd-9h64d" --- FAIL: TestIssue24_RemovedResourceGetsDeleted (25.20s) client.go:57: resource type GitOpsConfig with namespace/name (test-eunomia-operator/gitops-issue24) created util.go:97: Waiting for full availability of hello-world-issue24 pod util.go:97: Waiting for full availability of hello-world-issue24 pod util.go:97: Waiting for full availability of hello-world-issue24 pod util.go:114: pod hello-world-issue24 in namespace test-eunomia-operator is available util.go:114: pod now-only in namespace test-eunomia-operator is available issue24_test.go:91: Operation cannot be fulfilled on gitopsconfigs.eunomia.kohls.io "gitops-issue24": the object has been modified; please apply your changes to the latest version and try again This commit fixes the error on my machine, by refreshing the GitOpsConfig object before updating it in the cluster. My understanging is, that the object in the test was getting stale, because of the new Status field being added into it in the cluster between the Create and Update calls.
|
I added a proposal for a change that seems to fix it on my local machine, see here: https://github.com/safirh/eunomia/pull/1 I believe the Sleep increase should not be needed with this. |
test/e2e: refresh CR before updating it
|
Thanks @akavel ... your changes fixes the travis build error |
3 participants
Description
Status section added to the GitOpsConfig, so that end users can view the status of the k8s Jobs that get run when applying config.
Fixes (#101)
Type of change