Skip to content
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

[v2-10-test] Backport pull_request_target removal #45527

Open
wants to merge 1 commit into
base: v2-10-test
Choose a base branch
from

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented Jan 9, 2025

This is a bulk change that synchronizes dev/ci scripts for v2-10-test branch with main #45266 - including follow-ups.

Rather than cherry-picking relevant PRs, this one gets the latest version of the scripts from main and updates the branch with some changes to adapt them to v2-10-test (such as bringing back python 3.8 support, removing some providers checks after the bulk move of providers and making sure all tests are passing.

This is far easier than cherry-picking the changes, because for the v2-10-test we stopped cherry-picking CI changes which was deemed unnecessary (we used to do it for all previous branches) but this made it far more difficult (if not impossible) to cherry-pick individual changes.

Fortunately, the CI scripts are maintained in the way that their latest version should in principle work for a v2-* branch and hopefully after just a few adjustments we should be able to synchronize the changes from main by updating all relevant CI/DEV scripts, dockerfile images, workflows, pre-commits etc.


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

@potiuk potiuk force-pushed the apply-workflow-run-removal branch 2 times, most recently from 7fa5856 to 338d156 Compare January 10, 2025 17:31
@potiuk potiuk force-pushed the apply-workflow-run-removal branch 14 times, most recently from 6dbc0af to c6b7c5a Compare January 11, 2025 19:04
@potiuk potiuk marked this pull request as ready for review January 11, 2025 19:07
@potiuk
Copy link
Member Author

potiuk commented Jan 11, 2025

Hey here. I know this one is huge and difficult to review, but this was the easiest way I could bring the "pull_request_target" removal to v2-10-test branch.

Since we stopped cherry-picking breeze changes to v2-10-test and made a LOT of chenges in main (removing Python 3.8, moving providers, adding test_sdk and so on - cherry-picking individual commits was not an option. So I choose a different path - I copuied the latest breeze, ci_scripts, Dockerfiles and .pre-commits and adapted them tov2-10-test - mostly removing stuff that was not needed in v2-10-test (providers, charts, new api etc. etc., adding back Python 3.8).

All other changes were results of fixing the tests.

I think the easiest way to review it is two-fold:

  1. you can compare all the breeze/ci stuff with main - and see the differences (mostly removals of the things above)
  2. then you can compare "airflow" and "tests" with v2-10-test and see that they only changed to accomodate to some tests scripts changes.

I know I am asking a lot, but this is the easiest way we can remove last remnants of `pull_request_target" - which is still a potential security issue.

@potiuk potiuk force-pushed the apply-workflow-run-removal branch from c6b7c5a to 1eed775 Compare January 11, 2025 19:17
@potiuk potiuk force-pushed the apply-workflow-run-removal branch from 1eed775 to 613c3e6 Compare January 11, 2025 20:27
Copy link
Member

@gopidesupavan gopidesupavan left a comment

Choose a reason for hiding this comment

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

Thanks Jarek, Nice one. seems some provider changes pulled, i think thats okay?

This is a bulk change that synchronizes dev/ci scripts for v2-10-test
branch with main #45266 - including follow-ups.

Rather than cherry-picking relevant PRs, this one gets the
latest version of the scripts from main and updates the branch with
some changes to adapt them to v2-10-test (such as bringing back
python 3.8 support, removing some providers checks after the
bulk move of providers and making sure all tests are passing.

This is far easier than cherry-picking the changes, because for
the v2-10-test we stopped cherry-picking CI changes which was
deemed unnecessary (we used to do it for all previous branches)
but this made it far more difficult (if not impossible) to
cherry-pick individual changes.

Fortunately, the CI scripts are maintained in the way that their
latest version **should** in principle work for a v2-* branch and
hopefully after just a few adjustments we should be able to
synchronize the changes from main by updating all relevant
CI/DEV scripts, dockerfile images, workflows, pre-commits etc.

Add actions in codeql workflows to scan github workflow actions (#45534)

* add actions in codeql workflows to scan github workflow actions

* add actions in codeql workflows to scan github workflow actions

CodeQL scanning can run always on all code (#45541)

The CodeQL scannig is fast and having custom configuration to
select which scanning to run should be run makes it unnecessarily
complex

We can just run all CodeQL scans always.

This has been suggested by actions codeql scan itself.

Add explicit permissions for all workflow-run workflows (#45548)

Those workflows inherit permissions from the calling workflows
but it's good to add explicit permissions to indicate what is
needed and in case we will also use the workflows for other purposes
in the future - default permissions for older repos might be
write so it's best to be explicit about the permissions.

Found by CodeQL scanning

Remove contents: write permission from generate-constraints (#45558)

The write permission cannot be set for PRs from forks in the
call workflow - so we have to come back to implicit permissions
and make explicit permissions passing a bit differently.

(cherry picked from commit ae32ebc)

Bump trove-classifiers from 2025.1.7.14 to 2025.1.10.15 (#45561)

Bumps [trove-classifiers](https://github.com/pypa/trove-classifiers) from 2025.1.7.14 to 2025.1.10.15.
- [Release notes](https://github.com/pypa/trove-classifiers/releases)
- [Commits](pypa/trove-classifiers@2025.1.7.14...2025.1.10.15)

---
updated-dependencies:
- dependency-name: trove-classifiers
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
(cherry picked from commit f3fd262)

Add optional --image-file-dir to store loaded files elsewhere (#45564)

While backorting the "pull_request_target" removal to v2-10-test
branches it turned out that there is not enough disk space
on Public runner to load all 5 images and keep the file dump at
the same time in the same filesystem. This PR allows to choose
where the load/save files will be stored and in the github
runner environment we store the files in "/mnt" wnich is a separate
folder with 40GB free.

(cherry picked from commit 6628049)

Fix --from-pr feature for image load and stabilize help

This is a follow-up after #45564 - it fixes the `--from-pr` and
`--from-run` to work (it was failing with file does not exist).

Also found out that gettempdir might return different directory
depending on which is your designated tmp directory (for example
in MacOS this is is a longer path in /var/.....) - so we have
to force the default during help generation to always return
"/tmp" so that the --help images do not change depending on which
system you are and what your tmp directory is.
@potiuk potiuk force-pushed the apply-workflow-run-removal branch from 613c3e6 to 58ccdd7 Compare January 11, 2025 21:30
@potiuk
Copy link
Member Author

potiuk commented Jan 11, 2025

Yeah - some test failure fixes needed :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants