Page MenuHomeElementl

[2/2] Add helm schema for dagster compute log manager
ClosedPublic

Authored by rexledesma on Dec 22 2020, 7:55 PM.

Details

Summary

As title. Depends on D5745.

Test Plan

integration
bk
helm template

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.Dec 22 2020, 8:08 PM
Harbormaster failed remote builds in B23354: Diff 28412!
Harbormaster returned this revision to the author for changes because remote builds failed.Dec 28 2020, 10:18 PM
Harbormaster failed remote builds in B23507: Diff 28582!

TEST PLAN
integration
bk

the existing test suite won't exercise any of this new code and i don't see any new tests, I assume you did some manual testing at least?

python_modules/automation/automation/helm/schema/subschema/compute_log_manager.py
18–20

these are StringSource in dagster config space right? so should allow

secretKey:
  env: MY_ENV_KEY

which i am guessing this won't do

update helm schema to support StringSource fields in dagster.yaml

@alangenfeld I added integration to the test plan as a sanity check that the helm chart would still install now that the default values were now typed. As for manual testing, similar to the previous diff this was done through helm template.

rexledesma added inline comments.
python_modules/automation/automation/helm/schema/subschema/compute_log_manager.py
18–20

Hm, I've changed the ones that are string source to the correct schema. However for this azure compute log manager, it's typed as string which looks like a bug. https://sourcegraph.com/github.com/dagster-io/dagster/-/blob/python_modules/libraries/dagster-azure/dagster_azure/blob/compute_log_manager.py#L88-96

rexledesma marked an inline comment as done.

up

alangenfeld added inline comments.
helm/dagster/templates/helpers/_compute-log-manager.tpl
2–17

woof this kinda sucks, is this how we do StringSource else where?

This revision is now accepted and ready to land.Jan 8 2021, 10:09 PM
helm/dagster/templates/helpers/_compute-log-manager.tpl
2–17

This is the first place in the helm chart where a StringSource field is exposed in values.yaml - all other instances of env: XXX are defined by us in the dagster.yaml using environment variables we have defined in a configmap.

not sure how else to coerce StringSource correctly in the template