Page MenuHomeElementl

add secrets/config maps from the executor config to the run launcher config, rather than replacing them
ClosedPublic

Authored by dgibson on Aug 1 2021, 7:03 PM.

Details

Summary

This was a very good point on https://github.com/dagster-io/dagster/pull/4331 - if you specify a secret on the run launcher, secrets on the executor should be additive, not... replace-i-tive.

Test Plan

Integration

Diff Detail

Repository
R1 dagster
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Aug 1 2021, 7:20 PM
Harbormaster failed remote builds in B34531: Diff 42697!
Harbormaster returned this revision to the author for changes because remote builds failed.Aug 1 2021, 8:26 PM
Harbormaster failed remote builds in B34532: Diff 42698!
dgibson added a reviewer: johann.
johann requested changes to this revision.Aug 3 2021, 2:31 PM

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

integration_tests/test_suites/k8s-integration-test-suite/test_executor.py
140

What executor config are we leaving out for this test?

This revision now requires changes to proceed.Aug 3 2021, 2:31 PM

johann's feedback, beefed up the test coverage quite a bit

@alangenfeld or @rexledesma if you wouldn't mind reviewing this since johann is out that week I would be very grateful

rexledesma added inline comments.
python_modules/libraries/dagster-k8s/dagster_k8s/executor.py
86–93

the or [] is a bit odd - exc_cfg.get should return a list since it's typed by config, right?

This revision is now accepted and ready to land.Aug 11 2021, 9:36 PM
python_modules/libraries/dagster-k8s/dagster_k8s/executor.py
86–93

env_config_maps is Noneable so I believe it can be None, for better or worse

(was more relevant when it would replace the launcher config when it was set though - since in that case None was distinct from [])