Skip to content

fix: strip trailing slash of base url in config (#44337) #53884

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
dada-engineer wants to merge 1 commit into from
Closed

fix: strip trailing slash of base url in config (#44337) #53884

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions airflow-core/src/airflow/configuration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -777,6 +777,7 @@ def validate(self):
)

self._upgrade_postgres_metastore_conn()
self._strip_trailing_slash_from_base_url()
self.is_validated = True

def _upgrade_postgres_metastore_conn(self):
Expand Down Expand Up @@ -808,6 +809,19 @@ def _upgrade_postgres_metastore_conn(self):
old_env_var = self._env_var_name("core", key)
os.environ.pop(old_env_var, None)

def _strip_trailing_slash_from_base_url(self):
Copy link
Member

@ashb ashb Aug 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we want to strip it. Don't most places now need to ensure that it always exists? #54831 for example, and there were one or two more

Copy link
Contributor Author

@dada-engineer dada-engineer Aug 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is why I created this originally. So I checked several places and the only reference checking if it ends with / was in the api_server. So all the other places, like the property log_url of taskinstance model, just assume the base url is without a trailing slash. My reasoning then was ok lets just make this assumption hold true and not have this ugly if not base_url.endswith("/") check all over the places.

Edit: I am also fine with just closing this PR and using the other one, I just see this as source for error again.

"""Validate that api base_url config does not end with `/`."""
key = "base_url"
base_url = self.get("api", key, fallback="")
if base_url and base_url.endswith("/"):
self.upgraded_values[("api", key)] = base_url
new_base_url = base_url.rstrip("/")
self._update_env_var(section="api", name=key, new_value=new_base_url)
# if the old value is set via env var, we need to wipe it
# otherwise, it'll "win" over our adjusted value
old_env_var = self._env_var_name("core", key)
Copy link
Member

@ashb ashb Aug 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the core section? If the api section one is set that will always be used in preference to the core one.

Copy link
Contributor Author

@dada-engineer dada-engineer Aug 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I didin't know this. Saw this code in another config modifying code and thought I need this. I'll remove this then 👍🏻

os.environ.pop(old_env_var, None)

def _validate_enums(self):
"""Validate that enum type config has an accepted value."""
for (section_key, option_key), enum_options in self.enums_options.items():
Expand Down
4 changes: 2 additions & 2 deletions airflow-core/src/airflow/models/taskinstance.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -616,10 +616,10 @@ def to_runtime_ti(self, context_from_server) -> RuntimeTaskInstanceProtocol:
def log_url(self) -> str:
"""Log URL for TaskInstance."""
run_id = quote(self.run_id)
base_url = conf.get("api", "base_url", fallback="http://localhost:8080/")
base_url = conf.get("api", "base_url", fallback="http://localhost:8080")
map_index = f"/mapped/{self.map_index}" if self.map_index >= 0 else ""
try_number = f"?try_number={self.try_number}" if self.try_number > 0 else ""
_log_uri = f"{base_url}dags/{self.dag_id}/runs/{run_id}/tasks/{self.task_id}{map_index}{try_number}"
_log_uri = f"{base_url}/dags/{self.dag_id}/runs/{run_id}/tasks/{self.task_id}{map_index}{try_number}"

return _log_uri

Expand Down
Loading