Use in-tree metav1.Condition instead of custom copied type #197
Conversation
k8s-ci-robot
added
cncf-cla: yes
| // TODO(adrianludwin) Is the following statement really correct? | ||
| // Set time as an obviously wrong value 1970-01-01T00:00:00Z since we | ||
| // overwrite conditions every time. | ||
| LastTransitionTime: metav1.Unix(0, 0), |
There was a problem hiding this comment.
I'd be happy to just leave that out since I assume it's the zero value anyway. Also SetStatusCondition seems to set it automatically if it's zero-valued anyway.
There was a problem hiding this comment.
Hmmm, removing it seems to introduce failing tests. I will have to investigate a bit more or leave it as it is now. WDYT? There are two quite different use-cases calling this function: loadNamespaceConditions and syncConditions. I suspect we need two functions with a bit different semantics to fix the failing tests.
There was a problem hiding this comment.
ok maybe just update the comment (that this code doesn't work without this statement) and I'll approve?
There was a problem hiding this comment.
Done as suggested. I think you could take a look. It seems to touch some of the core functions in HNC that I do not know very well, at least not yet.
k8s-ci-robot
added
the
ok-to-test
This is a refactoring. K8s 1.20 is EOL, so it should be safe to use the master type from api-machinery.
Tested: Ran both unit-tests ('make test') and integration test ('make test-e2e') successfully.
erikgb
force-pushed
the
refactor/conditions
branch
from
57d4749 to
8bddee6
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adrianludwin, erikgb The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
lgtm
This is a refactoring. K8s 1.20 is EOL, so it should be safe to use the master type from api-machinery.
Tested: Ran both unit-tests ('make test') and integration test ('make test-e2e') successfully.
/cc @adrianludwin
/cc @rjbez17