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

WIP: Feat s3 endpoint #6

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

kingdonb
Copy link
Member

@kingdonb kingdonb commented Nov 24, 2018

Includes changes from teamhephy/workflow#71 and teamhephy/workflow#74, #3, #5

This seems to be working well for me with DigitalOcean Spaces!

(There are some commits on the end that need to be squashed and made to conform to the style guide, marking this PR as wip...)

Also requires the changes in teamhephy/workflow#75, with a Values.global.s3.use_sse setting

@duanhongyi
Copy link
Member

duanhongyi commented Nov 25, 2018

@kingdonb

docker build fails

docker build -t /deis/postgres:git-01e7242 .

Modify the comment, and then I close my RP.

@duanhongyi
Copy link
Member

Why use the global use_sse variable?

@yebyen
Copy link

yebyen commented Nov 25, 2018

I think that only global variables are passed into depended charts... Try it, I got an error message from helm without the global

See kingdonb#1 (comment)

@duanhongyi
Copy link
Member

Hello everyone!
I reorganized the submission and replaced the global variable with a local use_sse #7

@kingdonb
Copy link
Member Author

Thank you, I see this makes sense. Will test and review.

@Cryptophobia
Copy link
Member

Thank you, I see this makes sense. Will test and review.

Thanks for testing @kingdonb . My plan is to merge these S3 support changes by @rwos in next release of Hephy v2.21 or to merge them into v2.20 as long as they are non-breaking changes.

@kingdonb
Copy link
Member Author

kingdonb commented Nov 26, 2018

@Cryptophobia That sounds good, I'd like to verify that the switch of base images to postgres:11 (removing ca-certificates) hasn't broken any behaviors in AWS S3, GCS, or Azure Blob, the currently supported storage APIs. I'm not sure if anyone else is testing database/postgres WAL backups.

Either that, or a hotfix to the current beta release adding "ca-certificates" – in the event that it is broken, I think the remainder of the S3_ENDPOINT things are still needing some changes and not ready to merge yet. But since I'm not usually testing on AWS anymore, I can't say for sure that it's broken in master right now either. (I have a suspicion but don't know how certificate verification works in Boto, so it's possible that everything is fine. My testing is very caveman-like, I'm not able to say for sure that any needed PRs to complete this are actively missing, only that I have not tested them all together and wound up with a perfectly functioning cluster.) Edit: It looks like I should have just looked in master, the latest image built for v2.7.0 postgres does actually include ca-certificates

I'll repeat the testing I've done on a supported cloud with v2.20-beta to be sure it's ready for release, I think that's the right way forward. We should be able to guarantee the S3_ENDPOINT changes are non-breaking for v2.20.1, in fact one better, we should guarantee that upgrading + reusing v2.20.0 values.yaml that omits the endpoint config entirely doesn't change any behavior or ruin any part of the configuration in the upgrade.

@Cryptophobia
Copy link
Member

Please see comment in the postgres issue #8 (comment). I think ca-certificates is still being updated on the image due to step #4 of the Dockerfile.

@Cryptophobia
Copy link
Member

I think the remainder of the S3_ENDPOINT things are still needing some changes and not ready to merge yet.

Agreed. We need to test the S3_ENDPOINT changes more.

in fact one better, we should guarantee that upgrading + reusing v2.20.0 values.yaml that omits the endpoint config entirely doesn't change any behavior or ruin any part of the configuration in the upgrade.

Totally agree with this.

@duanhongyi
Copy link
Member

duanhongyi commented Nov 27, 2018

@kingdonb hi, can you merge RP (#7)? It contains all commit of this; fixes bugs in build and uses related local variables instead of global variables.

Or instead of merge the RP, use it directly (#7) @Cryptophobia

@kingdonb
Copy link
Member Author

We are considering them for v2.21 @duanhongyi – the alpine change also might be able to get merged for 2.20 because it can help to save so much space in terms of docker images.

But the S3_ENDPOINT changes are still under consideration. #7 includes all of those, so if you can submit a new PR with only Alpine and not including S3_ENDPOINT, it might become the next release.

@kingdonb
Copy link
Member Author

I am testing this evening... v2.20-beta with Azure blob storage and AKS

@duanhongyi
Copy link
Member

@kingdonb That's RP (#5).

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.

6 participants