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

CI script to check template fields #42230

Conversation

gopidesupavan
Copy link
Contributor

Adding CI script to check template fields in provider operators and sensors


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

@gopidesupavan
Copy link
Contributor Author

gopidesupavan commented Sep 14, 2024

cc: @ferruzzi

A recent update to the #41848 template field caused a system test failure. To prevent similar issues, I started adding tests to verify all template fields. As suggested by ferruzzi in the discussion #42183 (comment), implementing a CI check would be a valuable addition to this process, so tried use ast and import modules approach.

I am happy to take some suggestion and any alternatives :).

@gopidesupavan
Copy link
Contributor Author

@potiuk Could you pleas help where to configure this script ?, Not sure how to configure this to CI level :) , I tried using a pre-commit hook, but it's encountering dependency issues. I've moved the script to a in_container to resolve these dependencies. or could you please direct me where to place this?

@gopidesupavan
Copy link
Contributor Author

cc: @o-nikolas

@gopidesupavan
Copy link
Contributor Author

Have another question at present it reads the providers yaml files and validates all the template fields from operators/sensors, is it good idea to check all the providers everytime part of pre-commit ? or only providers changed part of that commit? Please suggest, what would be the best approach?

@gopidesupavan
Copy link
Contributor Author

gopidesupavan commented Sep 14, 2024

Am able to figure-out how to configure in pre-commit script with existing references, thank you really easy to refer and workout 😄

Copy link
Contributor

@o-nikolas o-nikolas left a comment

Choose a reason for hiding this comment

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

I assume you manually tested this an it's all working as expected?

Comment on lines 36 to 41
if cmd_result.returncode != 0 and os.environ.get("CI") != "true":
console.print(
"\n[yellow]If you see strange stacktraces above, especially about missing imports "
"run this command:[/]\n"
)
console.print("[magenta]breeze ci-image build --python 3.8 --upgrade-to-newer-dependencies[/]\n")
Copy link
Contributor

Choose a reason for hiding this comment

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

We have this in a few places. Can we not just pop this into run_command_via_breeze_shell itself?

Copy link
Contributor Author

@gopidesupavan gopidesupavan Sep 16, 2024

Choose a reason for hiding this comment

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

correct, let me check if am able to move some of these kind blocks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated, I hope this is fine? moved these checks inside to common pre-commit utils.

@gopidesupavan
Copy link
Contributor Author

gopidesupavan commented Sep 16, 2024

I assume you manually tested this an it's all working as expected?

Yes , sorry i would have posted some screenshots :)

image

Error field operators:

image image image

I hope the error message details is fine?

Copy link
Contributor

@ferruzzi ferruzzi left a comment

Choose a reason for hiding this comment

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

😍

Other than Niko's suggestion for moving some duplicate code, and the need to rebase, I love it. Thanks for taking care of this!

I saw you tagged Jarek with a question; he's going to be away for a couple weeks.

@gopidesupavan
Copy link
Contributor Author

gopidesupavan commented Sep 17, 2024

😍

Other than Niko's suggestion for moving some duplicate code, and the need to rebase, I love it. Thanks for taking care of this!

I saw you tagged Jarek with a question; he's going to be away for a couple weeks.

Thank you 😄 , sorry i didnt know Jarek away, have resolved it looking at some references in pre-commit config :).

@gopidesupavan
Copy link
Contributor Author

Can i get some help please? tests are failing , not sure exactly is it due to any intermittent issues in pipeline.

* add pgbouncer and statsd ingress

* convert pgbouncer to PgBouncer

* improve tests, rename flower

* change ports

* fix tests
loucyx and others added 11 commits September 17, 2024 19:32
* 🚨 new ESLint rules and configuration.

* 🚨 fix all linting errors.

* 🚚 fix component casing.

* 🔧 add new import sorting for prettier.

* 🚨 apply new sorting of imports.

* 🔧 add missing `noUncheckedIndexedAccess` setting to avoid potential mistakes with index access.

* 📄 add license to `prettier.config.js`

* 🎨 format code examples to use spaces instead of tabs.

* 🎨 format openapi-gen files.

* 🔧 exclude `pnpm-lock.yaml` from adding a license.

* 🔧 add ignore of `pnpm-lock.yaml` in more places.
🌐 add statics word.

* 🔧 add `.vscode/settings.json`

* 🔧 add `*.tsbuildinfo` to `.gitignore`.
⬇️ downgrade typescript.

* 🔧 revert `.vscode` files ignore in gitignore.
🔥 remove `settings.json` for now.

* ⏪ revert formatting in .pre-commit-config.yaml

* 🚚 change casing of files to match main.

* 🔀 fix main merge issues -_-

* 🚚 fix incorrect main file casing change.

* 🔥 delete duplicated files that were moved in main.

* 🎨 add missing space in pre-commit-config

* ✏️ fix pnpm-lock.yaml

* 🔧 fix dist ignoring.

* 🤔 Lint and fox vite.config.ts.
* deprecatd kerberos auth airflow.api.auth.backend.kerberos_auth and airflow.auth.managers.fab.api.auth.backend.kerberos_aut removed

* news fragment added

* deprecatd kerberos auth airflow.api.auth.backend.kerberos_auth and airflow.auth.managers.fab.api.auth.backend.kerberos_aut removed
…mitHook (apache#40757)

* Add: added kerberos related connection fields(principal, keytab)

* Update airflow/providers/apache/spark/hooks/spark_submit.py

* Update airflow/providers/apache/spark/hooks/spark_submit.py

* fixed wrong logic since keytab priority

* fixed wrong temp_file_name logic

* Added annotations on operators/spark_submit.py

* changed raise Exception to AirflowException

* Fixed principal referencing bug

* Added checksum validating logic to do not rewrite keytab

* Fixed arg-type error

* simplified existing keytab compare logic

* Added principal test cases

* Added keytab test cases

* added principal & keytab to expected conn values

* added principal & keytab getter for the borken conneciton case

* added guard logic for the case that failed to b64decode

* removed useless logic on test code

* Mod: improved keytab_value_override test case

* changed func name _get_keytab_from_base64 to _create_keytab_path_from_base64_keytab

* added additional comment on create_keytab parts

* added base64 decode exception test case

* added keytab write & move exception test case

* removed _get_keytab & _get_principal

* rollback _get_keytab & _get_principal

* set default values of keytab & principal on conn_data

---------

Co-authored-by: Youngha, Park <[email protected]>
…pache#40191) (apache#42268)

* fix(helm): render global volumes and volume mounts in cleanup job (#64056)

* fix(helm): allow to deactivate cleanup job history (#64056)
Bumps [dompurify](https://github.com/cure53/DOMPurify) from 2.2.9 to 2.5.6.
- [Release notes](https://github.com/cure53/DOMPurify/releases)
- [Commits](cure53/DOMPurify@2.2.9...2.5.6)

---
updated-dependencies:
- dependency-name: dompurify
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@gopidesupavan
Copy link
Contributor Author

Sorry i messed up something while rebasing

@gopidesupavan
Copy link
Contributor Author

Is it okay if i create new clean PR with referencing this one, due to some issues while doing rebase all the previous merged commits again showing in changes 😞 .

@dstandish
Copy link
Contributor

Is it okay if i create new clean PR with referencing this one, due to some issues while doing rebase all the previous merged commits again showing in changes 😞 .

go for it

@gopidesupavan
Copy link
Contributor Author

Is it okay if i create new clean PR with referencing this one, due to some issues while doing rebase all the previous merged commits again showing in changes 😞 .

go for it

Thank you. created new pr.

@vincbeck
Copy link
Contributor

Close in favor of #42284

@vincbeck vincbeck closed this Sep 18, 2024
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.