fat-catTW commented on PR #72499: URL: https://github.com/apache/airflow/pull/72499#issuecomment-5882528337
I like the goal of moving this closer to the response path, especially because it reduces the chance that a future DagRun endpoint forgets to mask `conf`. My hesitation is mostly around using a `model_validator` with `dag` passed through Pydantic context. This masking depends on the Dag version for that specific run, not just on fields already present in `DAGRunResponse`. The `dag_id` cache issue we just fixed is a good example of how easy it is to get that wrong in a subtle way. So I think `VariableResponse.redact_val()` is slightly different: it can decide what to redact from `key` and `val` on the model itself. For DagRun `conf`, we need an externally resolved Dag/schema. I’d lean toward centralizing this, but keeping the Dag dependency explicit. Something like a shared helper/factory, e.g. `build_masked_dag_run_response(dag_run, dag)` or `DAGRunResponse.from_dag_run(dag_run, dag=...)`, feels like a nice middle ground to me. It still gives us one place for the masking logic, while making it clear at each call site which Dag/version we are using. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
