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
Unknown Object (File)
Thu, May 11, 4:34 PM
Unknown Object (File)
Thu, May 11, 2:46 AM
Unknown Object (File)
Wed, May 10, 10:44 AM
Unknown Object (File)
Wed, May 10, 8:57 AM
Unknown Object (File)
May 9 2023, 3:51 AM
Unknown Object (File)
May 7 2023, 2:16 PM
Unknown Object (File)
May 4 2023, 10:26 PM
Unknown Object (File)
Apr 23 2023, 8:06 PM
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
Branch
addstuff (branched from master)
Lint
Lint Passed
Unit
No Test Coverage

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
136

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