-
Notifications
You must be signed in to change notification settings - Fork 16.3k
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.
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
fix: strip trailing slash of base url in config (#44337) #53884
Conversation
6656129 to
33398e3
Compare
|
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. |
33398e3 to
97f9b53
Compare
| old_env_var = self._env_var_name("core", key) | ||
| os.environ.pop(old_env_var, None) | ||
|
|
||
| def _strip_trailing_slash_from_base_url(self): |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
| 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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 👍🏻
The `RuntimeTaskInstance.log_url` property incorrectly concatenated base_url with the path, causing malformed URLs when base_url didn't end with a slash. Fixes apache#54247 Closes apache#54248 Closes apache#53884
…5699) The `RuntimeTaskInstance.log_url` property incorrectly concatenated base_url with the path, causing malformed URLs when base_url didn't end with a slash. Fixes apache#54247 Closes apache#54248 Closes apache#53884
…5699) The `RuntimeTaskInstance.log_url` property incorrectly concatenated base_url with the path, causing malformed URLs when base_url didn't end with a slash. Fixes apache#54247 Closes apache#54248 Closes apache#53884
…5699) The `RuntimeTaskInstance.log_url` property incorrectly concatenated base_url with the path, causing malformed URLs when base_url didn't end with a slash. Fixes apache#54247 Closes apache#54248 Closes apache#53884
…5699) The `RuntimeTaskInstance.log_url` property incorrectly concatenated base_url with the path, causing malformed URLs when base_url didn't end with a slash. Fixes apache#54247 Closes apache#54248 Closes apache#53884
…5699) The `RuntimeTaskInstance.log_url` property incorrectly concatenated base_url with the path, causing malformed URLs when base_url didn't end with a slash. Fixes apache#54247 Closes apache#54248 Closes apache#53884
…5699) The `RuntimeTaskInstance.log_url` property incorrectly concatenated base_url with the path, causing malformed URLs when base_url didn't end with a slash. Fixes apache#54247 Closes apache#54248 Closes apache#53884
…5699) The `RuntimeTaskInstance.log_url` property incorrectly concatenated base_url with the path, causing malformed URLs when base_url didn't end with a slash. Fixes apache#54247 Closes apache#54248 Closes apache#53884
…5699) The `RuntimeTaskInstance.log_url` property incorrectly concatenated base_url with the path, causing malformed URLs when base_url didn't end with a slash. Fixes apache#54247 Closes apache#54248 Closes apache#53884
…5699) The `RuntimeTaskInstance.log_url` property incorrectly concatenated base_url with the path, causing malformed URLs when base_url didn't end with a slash. Fixes apache#54247 Closes apache#54248 Closes apache#53884
…5699) The `RuntimeTaskInstance.log_url` property incorrectly concatenated base_url with the path, causing malformed URLs when base_url didn't end with a slash. Fixes apache#54247 Closes apache#54248 Closes apache#53884
…5699) The `RuntimeTaskInstance.log_url` property incorrectly concatenated base_url with the path, causing malformed URLs when base_url didn't end with a slash. Fixes apache#54247 Closes apache#54248 Closes apache#53884
…5699) The `RuntimeTaskInstance.log_url` property incorrectly concatenated base_url with the path, causing malformed URLs when base_url didn't end with a slash. Fixes apache#54247 Closes apache#54248 Closes apache#53884
…5699) The `RuntimeTaskInstance.log_url` property incorrectly concatenated base_url with the path, causing malformed URLs when base_url didn't end with a slash. Fixes apache#54247 Closes apache#54248 Closes apache#53884
…5699) The `RuntimeTaskInstance.log_url` property incorrectly concatenated base_url with the path, causing malformed URLs when base_url didn't end with a slash. Fixes apache#54247 Closes apache#54248 Closes apache#53884
…5699) The `RuntimeTaskInstance.log_url` property incorrectly concatenated base_url with the path, causing malformed URLs when base_url didn't end with a slash. Fixes apache#54247 Closes apache#54248 Closes apache#53884
…5699) The `RuntimeTaskInstance.log_url` property incorrectly concatenated base_url with the path, causing malformed URLs when base_url didn't end with a slash. Fixes apache#54247 Closes apache#54248 Closes apache#53884
…5699) The `RuntimeTaskInstance.log_url` property incorrectly concatenated base_url with the path, causing malformed URLs when base_url didn't end with a slash. Fixes apache#54247 Closes apache#54248 Closes apache#53884
…5699) The `RuntimeTaskInstance.log_url` property incorrectly concatenated base_url with the path, causing malformed URLs when base_url didn't end with a slash. Fixes apache#54247 Closes apache#54248 Closes apache#53884
…5699) The `RuntimeTaskInstance.log_url` property incorrectly concatenated base_url with the path, causing malformed URLs when base_url didn't end with a slash. Fixes apache#54247 Closes apache#54248 Closes apache#53884
…5699) The `RuntimeTaskInstance.log_url` property incorrectly concatenated base_url with the path, causing malformed URLs when base_url didn't end with a slash. Fixes apache#54247 Closes apache#54248 Closes apache#53884
The `RuntimeTaskInstance.log_url` property incorrectly concatenated base_url with the path, causing malformed URLs when base_url didn't end with a slash. Fixes #54247 Closes apache/airflow#54248 Closes apache/airflow#53884 (cherry picked from commit 3e1638250fb8f58a399ad8c5946dbb77a079f63d) GitOrigin-RevId: 09560358e062f238e89018dc45dba44fb382b747
related: #44337
closes: #54247
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.