Should dedup the the lists? Or at least test behavior with duplicates (I think that's already happening in the integration test?)

lgtm

Do we support termination? Dagit makes that api call if so

users might still run into this issue when their image contains an ENTRYPOINT.

Good stuff

up

remove config defaults

Would make sense to move to the DagsterKubernetesClient, that said we don't currently use it directly in either of the k8s launchers or the celery k8s executor. So I think it's a general refactor to move our classes away from using the k8s lib directly

Excellent. Could consider mentioning 3rd party solutions, e.g.

Changed approach to log from the step_delegating_executor rather than the step handlers, because the executor has the step contexts and can log correctly. this also means you don't have to remember to log the individual events.

change approach

nice

Wow I had forgotten this was necessary

Awesome! Could move description from the diff to code comments

Read only dagit would be an example of a second dagit (with a new url) pointed at the same instance

breaking changes section

feedback

If this happened enough on k8s via CD or something, k8s might go into crashloopbackoff. Looks like that's not tunable https://github.com/kubernetes/kubernetes/issues/57291. But users probably shouldn't hit it...
If they do, it's an exponential backoff capped at 5 min and clears after 10 min of successful running. Seems ok

Going to discuss offline

Awesome! I think this will deserve at least a callout in the #dagster-kubernetes channel, it's a decent quality-of-life improvement.

I like this direction too.

up

up

test

up

feedback

Should we consider raising an error when .get is called outside of a context manager? Something like https://stackoverflow.com/a/54514410/14656695

up

up

up

up

up