Skip to content

Conversation

amoghrajesh
Copy link
Contributor

@amoghrajesh amoghrajesh commented Jul 9, 2025

closes: #53033

Came across a case in the issue attached where the base_url was set to empty and due to us not handling it well enough, it was getting through to later stages and resulting in execution API url being /executionwhich is not a good user experiences.

The hidden issue is that conf.get returns an empty string "" instead of using the fallback value when empty string is set. This could be corrected over time, but is not needed as of now as it could have hidden effects and in most cases it is handled better further down.

Whatever the reason might be, the supervisor must account for this, and throw a better error message than just failing.

I am handling the case here by checking for valid URLs before creating the SDK client.

image

When its not provided.


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in airflow-core/newsfragments.

@amoghrajesh amoghrajesh self-assigned this Jul 9, 2025
@amoghrajesh amoghrajesh added this to the Airflow 3.0.4 milestone Jul 9, 2025
@amoghrajesh amoghrajesh added the backport-to-v3-0-test Mark PR with this label to backport to v3-0-test branch label Jul 9, 2025
Copy link
Member

@kaxil kaxil left a comment

Choose a reason for hiding this comment

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

hmm, when can this happen?

If the users sets it to empty string by themselves, that is most certainly a user error

@amoghrajesh amoghrajesh force-pushed the handle-empty-baseurl branch from 1a3f7ca to 6f0d6ee Compare July 10, 2025 08:48
@amoghrajesh amoghrajesh changed the title Handle cases when base_url is set to empty in the config Handle invalid execution API urls in supervisor Jul 10, 2025
@amoghrajesh
Copy link
Contributor Author

@kaxil what do you think of this approach? Makes more sense ig

@amoghrajesh amoghrajesh requested a review from kaxil July 10, 2025 08:57
@amoghrajesh
Copy link
Contributor Author

@kaxil could you take a look at this when you have some time?

@amoghrajesh
Copy link
Contributor Author

This is a bugfix, can go in 3.0.4

@amoghrajesh amoghrajesh merged commit e9e6ac2 into apache:main Jul 19, 2025
76 checks passed
@amoghrajesh amoghrajesh deleted the handle-empty-baseurl branch July 19, 2025 05:28
github-actions bot pushed a commit that referenced this pull request Jul 19, 2025
Copy link

Backport successfully created: v3-0-test

Status Branch Result
v3-0-test PR Link

github-actions bot pushed a commit to aws-mwaa/upstream-to-airflow that referenced this pull request Jul 19, 2025
potiuk pushed a commit that referenced this pull request Jul 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:Executors-core LocalExecutor & SequentialExecutor area:providers area:task-sdk backport-to-v3-0-test Mark PR with this label to backport to v3-0-test branch provider:celery
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LocalExecutor url resolution makes bad decisions regarding defaults
3 participants