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
session: ensure 'tilt ci' exits properly when reattaching to jobs #6276
Conversation
dc5e529
to
e8aa586
Compare
Signed-off-by: Daniel Nephin <dnephin@gmail.com>
fixes tilt-dev#6272 History of this change: - we used to have a very naive definition of "Ready" - callers of the runtime status API had hacks to look at Job pods to determine completeness (tilt-dev#4367) - later, we changed it to have a more sophisticated definition, so that Jobs only count as 'ready' when they're complete. (tilt-dev#5013) - we forgot to remove the old caller hacks :grimace: in any case, i added a bunch of tests, since we need more job integration tests anyway. Signed-off-by: Nick Santos <nick.santos@docker.com>
e8aa586
to
4d02ced
Compare
cc @dnephin |
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.
Thanks! LGTM
krs.HasEverDeployedSuccessfully = true | ||
// There are no pods but the job completed successfully. | ||
krs.Conditions = []metav1.Condition{ | ||
{ | ||
Type: v1alpha1.ApplyConditionJobComplete, | ||
Status: metav1.ConditionTrue, | ||
}, | ||
} |
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.
I think some or all of these lines are no longer necessary for the test to pass (but I haven't actually confirmed that by trying it).
krs.HasEverDeployedSuccessfully = true | |
// There are no pods but the job completed successfully. | |
krs.Conditions = []metav1.Condition{ | |
{ | |
Type: v1alpha1.ApplyConditionJobComplete, | |
Status: metav1.ConditionTrue, | |
}, | |
} |
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.
nope! these fields are used to infer runtime state, see
tilt/internal/store/runtime_state.go
Line 136 in 01b9ea6
if meta.IsStatusConditionTrue(s.Conditions, v1alpha1.ApplyConditionJobComplete) { |
fixes #6272
History of this change:
in any case, i added a bunch of tests, since we need more job integration tests anyway.