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

Bugfix/s3 website configuration #649

Merged

Conversation

jack1902
Copy link
Contributor

@jack1902 jack1902 commented May 4, 2021

Description of your changes

Slack Thread: https://crossplane.slack.com/archives/CEF5N8X08/p1620143654284900

Fixes #650

I have:

  • Read and followed Crossplane's contribution process
  • Run make reviewable test to ensure this PR is ready for review.

To Note, i have removed protocol as a required parameter because even the description states it defaults to what the inbound request is yet the Spec states its required which goes against the given description

How has this code been tested

  • I have been trying to use the website configuration on the bucket resource within a composition. I noticed that it worked if i passed routing rules but didn't when i only passed the index and error documents (things i have already done before in terraform but i am moving the bucket creation to crossplane 🚀 )

  • make test - ✅

  • make e2e - ✅
    Screenshot 2021-05-04 at 21 09 59

  • make reviewable - ✅

  • make - ✅

@jack1902 jack1902 force-pushed the bugfix/s3-website-configuration branch from b6b4eec to f619107 Compare May 4, 2021 19:30
Copy link
Member

@muvaf muvaf left a comment

Choose a reason for hiding this comment

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

Hi @jack1902 ! Congrats on your first contribution! One small caveat and I think we're good to go!

@jack1902 jack1902 requested a review from muvaf May 5, 2021 08:48
Copy link
Member

@muvaf muvaf left a comment

Choose a reason for hiding this comment

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

Just small comments. Make sure to squash the commits when you're done so that I can merge right away :)

pkg/controller/s3/bucket/websiteConfig.go Outdated Show resolved Hide resolved
pkg/controller/s3/bucket/websiteConfig.go Outdated Show resolved Hide resolved
@jack1902 jack1902 force-pushed the bugfix/s3-website-configuration branch from 23a2d11 to 11f8c8f Compare May 5, 2021 12:21
- Ensures protocol can be set blank as per the description
- Fixes the website config to work when no routing rules are passed

Signed-off-by: jack1902 <39212456+jack1902@users.noreply.github.com>
@jack1902 jack1902 force-pushed the bugfix/s3-website-configuration branch from 11f8c8f to 020825c Compare May 5, 2021 12:22
@jack1902 jack1902 requested a review from muvaf May 5, 2021 12:26
Copy link
Member

@muvaf muvaf left a comment

Choose a reason for hiding this comment

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

Thanks @jack1902 !

@muvaf muvaf merged commit 319c8f6 into crossplane-contrib:master May 5, 2021
@jack1902 jack1902 deleted the bugfix/s3-website-configuration branch May 5, 2021 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

S3 Website configuration won't work without routingRules
2 participants