pierrejeambrun commented on code in PR #31833:
URL: https://github.com/apache/airflow/pull/31833#discussion_r1225407444
##########
airflow/models/taskinstance.py:
##########
@@ -758,7 +758,7 @@ def generate_command(
def log_url(self) -> str:
"""Log URL for TaskInstance."""
iso = quote(self.execution_date.isoformat())
- base_url = conf.get_mandatory_value("webserver", "BASE_URL")
+ base_url = conf.get_mandatory_value("webserver",
"BASE_URL").rstrip("/")
Review Comment:
I feel like this could lead to some errors in the future.
It's hard for someone to guess that retrieving the `base_url` from the
config is a special case and needs to strip trailing '/'. (I can easily imagine
someone doing `conf.get_mandatory_value("webserver", "BASE_URL")` to access the
value)
Maybe we can put that in the config accessor ? (So we don't need to bother,
the equivalent of `@validates/@pre_process` for marshmallow field, that does
some preprocesssing on the value before storing it)
Otherwise looking good
--
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]
