-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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
Update GCS hook to get crc32c hash for CMEK-protected objects #34982
Conversation
…hat crc32c is included
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
|
Shouldn't this need a test against whatever that blob should be? |
Not sure I understand, could you please explain further or with an example? Thanks. |
sure, so the function that has been changed is here is a private function, so we can't test the logic that has been added but from what I can understand there should be some behavioral changes to the in terms of the data it is parsing from the blob and in my opinion there should be respective tests added to https://github.com/RNHTTR/airflow/blob/78d1031b34d70784d22fc1360c6f3deeb2c66a2c/airflow/providers/google/cloud/hooks/gcs.py so that the behavior is captured. |
Need to fix static checks and failed test |
Thanks, still figuring out how to write the test correctly. |
Are you worry about that you can't test internal static method |
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.
The test TestSyncGcsHook.test_should_overwrite_cmek_files
should be fixed.
Besides - LGTM.
@dmedora can you please fix the test? |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions. |
Closes: #34980
Updated _prepare_sync_plan function to get blobs using the Objects Get API (.get_blob(..)) so that their crc32c value is included in the returned Blob object. As discussed in #34980, the current Objects List method does not return this for CMEK-protected objects, leading to inaccurate hash comparisons.
^ 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.