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

[AIRFLOW-4175] S3Hook load_file should support ACL policy parameter #7726

Closed
wants to merge 1 commit into from

Conversation

OmairK
Copy link
Contributor

@OmairK OmairK commented Mar 14, 2020

- Added acl_policy parameter to all the S3Hook.load_*() and S3Hook.copy_object() functions
- Added unittest to test the response permissions when the policy is passed
- Updated the docstring of the function

Issue link: AIRFLOW-4175

Make sure to mark the boxes below before creating PR: [x]

  • Description above provides context of the change
  • Commit message/PR title starts with [AIRFLOW-NNNN]. AIRFLOW-NNNN = JIRA ID*
  • Unit tests coverage for changes (not needed for documentation changes)
  • Commits follow "How to write a good git commit message"
  • Relevant documentation is updated including usage instructions.
  • I will engage committers as explained in Contribution Workflow Example.

* For document-only changes commit message can start with [AIRFLOW-XXXX].


In case of fundamental code change, 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 UPDATING.md.
Read the Pull Request Guidelines for more information.

	- Added acl_policy parameter to all the S3Hook.load_*() and S3Hook.copy_object() functions
	- Added unittest to test the response permissions when the policy is passed
	- Updated the docstring of the function
@boring-cyborg boring-cyborg bot added the provider:amazon-aws AWS/Amazon - related issues label Mar 14, 2020
Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

It looks good. But one thing - can you also extend the tests to verify that the default "private" acl has also the right permissions ?

@OmairK
Copy link
Contributor Author

OmairK commented Mar 15, 2020

It looks good. But one thing - can you also extend the tests to verify that the default "private" acl has also the right permissions ?

You are talking about the copy_object() hook ?

@feluelle
Copy link
Member

I am sorry, but this is a duplicate. #7635 already adds ACL Header.

@OmairK
Copy link
Contributor Author

OmairK commented Mar 15, 2020

I am sorry, but this is a duplicate. #7635 already adds ACL Header.

@feluelle I have already completed the task and made the changes @potiuk suggested, and since this issue was not explicitly assigned to anybody I have been working on this issue for the past 2 days.

@potiuk
Copy link
Member

potiuk commented Mar 15, 2020

I think we can combine the two.. There are more tests in this one - so I think we should use it :). @retornam - how about you review this one instead of merging yours :)? Let's join forces.

@potiuk
Copy link
Member

potiuk commented Mar 15, 2020

@OmairK -> I think you will have to rebase and push the branch again so that we can re-open it.

@OmairK
Copy link
Contributor Author

OmairK commented Mar 15, 2020

@OmairK -> I think you will have to rebase and push the branch again so that we can re-open it.

@potiuk I have rebased and forced pushed the branch.

@potiuk
Copy link
Member

potiuk commented Mar 15, 2020

And BTW - both @OmairK and @retornam can be co-authors of the change https://help.github.com/en/github/committing-changes-to-your-project/creating-a-commit-with-multiple-authors - we've done that multiple times and this is true open-source spirit ... - so why don't you add @retornam co-author line to the change :)?

@potiuk
Copy link
Member

potiuk commented Mar 15, 2020

Somehow I can't re-open it :(. Likely you need to reopen it yourself @OmairK or create a new one with new branch name

@retornam
Copy link
Contributor

@OmairK I can close mine, when you submit a new PR.

@OmairK
Copy link
Contributor Author

OmairK commented Mar 15, 2020

And BTW - both @OmairK and @retornam can be co-authors of the change https://help.github.com/en/github/committing-changes-to-your-project/creating-a-commit-with-multiple-authors - we've done that multiple times and this is true open-source spirit ... - so why don't you add @retornam co-author line to the change :)?

Sure I will do that right now.

@OmairK I can close mine, when you submit a new PR.

Thanks @retornam 😄

@feluelle
Copy link
Member

Thank you @retornam and @OmairK for co-authoring this :) 👍

So #7733 is the new one and we can close #7635, right?

@potiuk
Copy link
Member

potiuk commented Mar 16, 2020

Right @feluelle !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
provider:amazon-aws AWS/Amazon - related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants