You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "dada-engineer (via GitHub)" <gi...@apache.org> on 2025/07/29 12:25:32 UTC
[PR] fix: strip trailing slash of base url in config (#44337) [airflow]
dada-engineer opened a new pull request, #53884:
URL: https://github.com/apache/airflow/pull/53884
related: #44337
The log url generated assumed a trailing slash in the base_url config, all other places don't. I think it should be validated and updated to strip these away and append when needed in airflow.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: strip trailing slash of base url in config (#44337) [airflow]
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on code in PR #53884:
URL: https://github.com/apache/airflow/pull/53884#discussion_r2304097022
##########
airflow-core/src/airflow/configuration.py:
##########
@@ -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):
+ """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)
Review Comment:
Why the core section? If the api section one is set that will always be used in preference to the core one.
##########
airflow-core/src/airflow/configuration.py:
##########
@@ -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):
Review Comment:
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
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: strip trailing slash of base url in config (#44337) [airflow]
Posted by "kaxil (via GitHub)" <gi...@apache.org>.
kaxil closed pull request #53884: fix: strip trailing slash of base url in config (#44337)
URL: https://github.com/apache/airflow/pull/53884
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: strip trailing slash of base url in config (#44337) [airflow]
Posted by "dada-engineer (via GitHub)" <gi...@apache.org>.
dada-engineer commented on code in PR #53884:
URL: https://github.com/apache/airflow/pull/53884#discussion_r2309160978
##########
airflow-core/src/airflow/configuration.py:
##########
@@ -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):
+ """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)
Review Comment:
Ah I didin't know this. Saw this code in another config modifying code and thought I need this. I'll remove this then 👍🏻
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: strip trailing slash of base url in config (#44337) [airflow]
Posted by "dada-engineer (via GitHub)" <gi...@apache.org>.
dada-engineer commented on code in PR #53884:
URL: https://github.com/apache/airflow/pull/53884#discussion_r2309159338
##########
airflow-core/src/airflow/configuration.py:
##########
@@ -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):
Review Comment:
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](https://github.com/apache/airflow/blob/main/airflow-core/src/airflow/models/taskinstance.py#L623), 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.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: strip trailing slash of base url in config (#44337) [airflow]
Posted by "dada-engineer (via GitHub)" <gi...@apache.org>.
dada-engineer commented on code in PR #53884:
URL: https://github.com/apache/airflow/pull/53884#discussion_r2309159338
##########
airflow-core/src/airflow/configuration.py:
##########
@@ -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):
Review Comment:
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](https://github.com/apache/airflow/blob/main/airflow-core/src/airflow/models/taskinstance.py#L623), 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.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: strip trailing slash of base url in config (#44337) [airflow]
Posted by "dada-engineer (via GitHub)" <gi...@apache.org>.
dada-engineer commented on PR #53884:
URL: https://github.com/apache/airflow/pull/53884#issuecomment-3132650366
FYI I belive there is already a test with the base url specified with a trailing slash that loads the config so I did not add a separate one additionally.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: strip trailing slash of base url in config (#44337) [airflow]
Posted by "kaxil (via GitHub)" <gi...@apache.org>.
kaxil commented on PR #53884:
URL: https://github.com/apache/airflow/pull/53884#issuecomment-3183648486
Related PRs:
- https://github.com/apache/airflow/pull/54218
- https://github.com/apache/airflow/pull/54248
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org