Skip to content

Conversation

@johnslavik
Copy link
Contributor

@johnslavik johnslavik commented Dec 2, 2025

Otherwise (i.e., if we use prek < 0.2.0) breeze start-airflow can't start Airflow from top-level as it relies on nested .pre-commit-config.yml discovery that's been introduced in prek 0.2.0 ("Workspace Mode").

I'll mark as ready after confirming the minimum version works like a breeze.


^ 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.

@johnslavik
Copy link
Contributor Author

skip common compat check I guess?

@johnslavik
Copy link
Contributor Author

🚀

image

@johnslavik
Copy link
Contributor Author

johnslavik commented Dec 2, 2025

When the prek version is lower than required, the error message from breeze start-airflow is now much more helpful:

(...)
SimpleAuthManager detected. All roles (admin, viewer, user, op) are always available via configuration in .dev/breeze/src/airflow_breeze/files/simple_auth_manager_passwords.json

The asset compilation failed. Exiting.

error: Failed to parse `.pre-commit-config.yaml`
  caused by: Required minimum prek version `0.2.0` is greater than current version `0.1.1`. Please consider updating prek. at line 18 column 1


Error 1 returned

(previously it was just No hook found with id `compile-ui-assets` in stage manual or something like that)

Maybe in a followup PR we could make breeze start-airflow fail early though?

In both scenarios the script assumes it's waiting for a prek job that started successfully and could fail, not for a job that didn't start (and I think we can dry-run / check that before launching everything else, since if prek fails then the entire machinery is destined to fail at some point -- otherwise we'd get a warning, not an error).

Not sure if it's worth it to fix that though -- maybe it's too much of an edge case?

@potiuk potiuk added the skip common compat check Skips common compat provider modification check label Dec 2, 2025
Otherwise hooks aren't discovered recursively via workspaces feature
Copy link
Contributor

@jscheffl jscheffl left a comment

Choose a reason for hiding this comment

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

Cool!

@potiuk potiuk merged commit b95fca4 into apache:main Dec 2, 2025
86 checks passed
@github-actions
Copy link

github-actions bot commented Dec 2, 2025

Backport failed to create: v3-1-test. View the failure log Run details

Status Branch Result
v3-1-test Commit Link

You can attempt to backport this manually by running:

cherry_picker b95fca4 v3-1-test

This should apply the commit to the v3-1-test branch and leave the commit in conflict state marking
the files that need manual conflict resolution.

After you have resolved the conflicts, you can continue the backport process by running:

cherry_picker --continue

@potiuk
Copy link
Member

potiuk commented Dec 2, 2025

In both scenarios the script assumes it's waiting for a prek job that started successfully and could fail, not for a job that didn't start (and I think we can dry-run / check that before launching everything else, since if prek fails then the entire machinery is destined to fail at some point -- otherwise we'd get a warning, not an error)

Yes - we could. We already do that for other things so checking for prek (but specifically when prek should be used) is a good idea.

https://github.com/apache/airflow/blob/main/dev/breeze/src/airflow_breeze/utils/docker_command_utils.py#L539

This is actually a very important point to fix those things and give as good of a message for all those edge cases as it makes it easier for new contributos. We usually discover always something when new person starts using things and we usually relentlessly fix it :)

@potiuk
Copy link
Member

potiuk commented Dec 2, 2025

@johnslavik

Actually - we even have the right method - we just do not call it in start-airflow . It even reads the version from .pre-commit-config.yaml

def assert_prek_installed():
    """
    Check if prek is installed in the right version.

    :return: True is the prek is installed in the right version.

potiuk pushed a commit to potiuk/airflow that referenced this pull request Dec 2, 2025
Otherwise hooks aren't discovered recursively via workspaces feature
(cherry picked from commit b95fca4)

Co-authored-by: Bartosz Sławecki <bartosz@slawecki.dev>
potiuk added a commit that referenced this pull request Dec 2, 2025
Otherwise hooks aren't discovered recursively via workspaces feature
(cherry picked from commit b95fca4)

Co-authored-by: Bartosz Sławecki <bartosz@slawecki.dev>
@potiuk
Copy link
Member

potiuk commented Dec 2, 2025

Backport in #58977

ephraimbuddy pushed a commit that referenced this pull request Dec 3, 2025
Otherwise hooks aren't discovered recursively via workspaces feature
(cherry picked from commit b95fca4)

Co-authored-by: Bartosz Sławecki <bartosz@slawecki.dev>
RoyLee1224 pushed a commit to RoyLee1224/airflow that referenced this pull request Dec 3, 2025
Otherwise hooks aren't discovered recursively via workspaces feature
itayweb pushed a commit to itayweb/airflow that referenced this pull request Dec 6, 2025
Otherwise hooks aren't discovered recursively via workspaces feature
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:airflow-ctl area:dev-tools area:go-sdk area:helm-chart Airflow Helm Chart area:providers area:task-sdk backport-to-v3-1-test Mark PR with this label to backport to v3-1-test branch provider:common-compat provider:edge Edge Executor / Worker (AIP-69) / edge3 provider:fab provider:keycloak skip common compat check Skips common compat provider modification check

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants