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.

Sign up for GitHub

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

Jump to bottom

Add expected bucket owner configuration #349

Merged
merged 6 commits into from
Jul 4, 2023
Merged

Add expected bucket owner configuration #349

merged 6 commits into from
Jul 4, 2023

Conversation

sum12
Copy link
Contributor

@sum12 sum12 commented Jun 30, 2023 •
edited by dannycjones
Loading

Description of change

#286

Does this change impact existing behavior?

No

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and I agree to the terms of the Developer Certificate of Origin (DCO).

@sum12 sum12 had a problem deploying to PR integration tests June 30, 2023 17:23 — with GitHub Actions Failure
@sum12 sum12 had a problem deploying to PR integration tests June 30, 2023 17:23 — with GitHub Actions Failure
@sum12 sum12 had a problem deploying to PR integration tests June 30, 2023 17:23 — with GitHub Actions Failure
@sum12 sum12 had a problem deploying to PR integration tests June 30, 2023 17:23 — with GitHub Actions Failure
@passaro passaro temporarily deployed to PR integration tests June 30, 2023 17:47 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests June 30, 2023 17:47 — with GitHub Actions Inactive
@passaro passaro had a problem deploying to PR integration tests June 30, 2023 17:47 — with GitHub Actions Failure
@passaro passaro temporarily deployed to PR integration tests June 30, 2023 17:47 — with GitHub Actions Inactive
@monthonk monthonk self-requested a review July 3, 2023 14:43
monthonk
monthonk requested changes Jul 3, 2023
View reviewed changes
Copy link
Contributor

@monthonk monthonk left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! I think we should extend the test a bit to also verify the http headers and a few nit-picking, but overall looks good to me.

mountpoint-s3/src/main.rs Outdated Show resolved Hide resolved
mountpoint-s3/src/main.rs Outdated Show resolved Hide resolved
mountpoint-s3-client/src/s3_crt_client.rs Outdated Show resolved Hide resolved
@sum12 @monthonk
Update mountpoint-s3/src/main.rs
a9d136f 
Co-authored-by: Monthon Klongklaew <[email protected]>
Signed-off-by: Sumit <[email protected]>
@sum12 sum12 had a problem deploying to PR integration tests July 4, 2023 09:10 — with GitHub Actions Failure
@sum12 sum12 had a problem deploying to PR integration tests July 4, 2023 09:10 — with GitHub Actions Failure
@sum12 @monthonk
Update mountpoint-s3/src/main.rs
292bd8e 
Co-authored-by: Monthon Klongklaew <[email protected]>
Signed-off-by: Sumit <[email protected]>
@sum12 sum12 had a problem deploying to PR integration tests July 4, 2023 09:10 — with GitHub Actions Failure
@sum12 sum12 had a problem deploying to PR integration tests July 4, 2023 09:10 — with GitHub Actions Failure
@sum12 sum12 had a problem deploying to PR integration tests July 4, 2023 09:10 — with GitHub Actions Failure
@sum12 sum12 had a problem deploying to PR integration tests July 4, 2023 09:10 — with GitHub Actions Failure
@sum12 @monthonk
Update mountpoint-s3-client/src/s3_crt_client.rs
fc8ec06 
Co-authored-by: Monthon Klongklaew <[email protected]>
Signed-off-by: Sumit <[email protected]>
@sum12 sum12 temporarily deployed to PR integration tests July 4, 2023 09:10 — with GitHub Actions Inactive
@sum12 sum12 temporarily deployed to PR integration tests July 4, 2023 09:10 — with GitHub Actions Inactive
@sum12 sum12 temporarily deployed to PR integration tests July 4, 2023 09:11 — with GitHub Actions Inactive
@sum12 sum12 had a problem deploying to PR integration tests July 4, 2023 09:11 — with GitHub Actions Failure
@dannycjones dannycjones temporarily deployed to PR integration tests July 4, 2023 12:30 — with GitHub Actions Inactive
@dannycjones dannycjones temporarily deployed to PR integration tests July 4, 2023 12:30 — with GitHub Actions Inactive
@dannycjones dannycjones temporarily deployed to PR integration tests July 4, 2023 12:30 — with GitHub Actions Inactive
@dannycjones dannycjones changed the title allow setting expected bucket owner and use it as condition for requests Add expected bucket owner configuration Jul 4, 2023
@dannycjones dannycjones requested a review from monthonk July 4, 2023 12:47
@dannycjones dannycjones temporarily deployed to PR integration tests July 4, 2023 12:53 — with GitHub Actions Inactive
@dannycjones dannycjones temporarily deployed to PR integration tests July 4, 2023 12:53 — with GitHub Actions Inactive
@dannycjones dannycjones temporarily deployed to PR integration tests July 4, 2023 12:53 — with GitHub Actions Inactive
@dannycjones dannycjones temporarily deployed to PR integration tests July 4, 2023 12:54 — with GitHub Actions Inactive
@dannycjones
Copy link
Contributor

I updated the argument definition for the placeholder and help text. I also merged in latest main commit so CI should be passing as expected.

@monthonk monthonk merged commit 780cc75 into awslabs:main Jul 4, 2023
@monthonk
Copy link
Contributor

monthonk commented Jul 4, 2023

merged, thank you!

@jamesbornholt jamesbornholt mentioned this pull request Jul 7, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dannycjones dannycjones dannycjones left review comments

@monthonk monthonk monthonk approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sum12 @dannycjones @monthonk @passaro