As title. Depends on D5745.
Details
Details
integration
bk
helm template
Diff Detail
Diff Detail
- Repository
- R1 dagster
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
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 | |
Comment Actions
@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.
| 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 | |
| helm/dagster/templates/helpers/_compute-log-manager.tpl | ||
|---|---|---|
| 2–17 | woof this kinda sucks, is this how we do StringSource else where? | |
| 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 | |