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.
Tags
None
Referenced Files
F2774066: D9178.diff
Sun, Feb 5, 2:15 AM
F2773666: D9178.id42859.diff
Sat, Feb 4, 11:33 AM
Unknown Object (File)
Sun, Jan 29, 7:04 PM
Unknown Object (File)
Dec 17 2022, 11:35 AM
Unknown Object (File)
Dec 13 2022, 10:42 PM
Unknown Object (File)
Nov 25 2022, 5:52 PM
Unknown Object (File)
Nov 19 2022, 3:03 PM
Unknown Object (File)
Nov 18 2022, 2:16 AM
Subscribers
None

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!

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 [])