-
Notifications
You must be signed in to change notification settings - Fork 1k
Fix cloning from GCS #1176
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
Fix cloning from GCS #1176
Conversation
update to latest
…e to the region it is in
…ocal time to the region it is in" This reverts commit ac4eb35.
👍 |
... | ||
``` | ||
|
||
### Setup pod environment configmap |
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.
I believe the documentation part needs to be formulated differently.
-
Title of the section should include "GCS" to indicate it's not just general configmap.
-
The operator doesn't have any preferences or suggestions about which method to use, so "we want to use WAL-G" need to be omitted. Instead it could be formulated as "To make operator work with GCS do the following: ...".
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.
this paragraph is part of a bigger section about GCP so title is fine.
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.
Indeed, then the title part is fine. But I would still ask to avoid indirect suggestions about using WAL-G (it could be a note though). Other than that looks good.
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.
@erthalion I re-worded that section based on your feedback. I left a note as to why we are using WAL-G instead of WAL-E, so that end-user understands the reasoning, instead of just using "magical" configmap. Let me know if you agree with the changes.
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.
@sagor999 Yes, looks good, thanks. Do you have any references/docs we can also add to make the claim about WAL-G suited better for GCS look more respectable?
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.
@erthalion valid point. I looked it up, and it seems like it is possible to make WAL-E work with GCS. So I have re-worded that phrase.
👍 |
1 similar comment
👍 |
* Fix clone from gcs * pass google credentials env var if using GS bucket * remove requirement for timezone as GCS returns timestamp in local time to the region it is in * Revert "remove requirement for timezone as GCS returns timestamp in local time to the region it is in" This reverts commit ac4eb35. * update GCS documentation * remove sentence about logical backups * reword pod environment configmap section * fix documentation
* Fix clone from gcs * pass google credentials env var if using GS bucket * remove requirement for timezone as GCS returns timestamp in local time to the region it is in * Revert "remove requirement for timezone as GCS returns timestamp in local time to the region it is in" This reverts commit ac4eb35. * update GCS documentation * remove sentence about logical backups * reword pod environment configmap section * fix documentation
This reverts commit cd3e327.
If using wal_gs_bucket, pass correct env variables for cloning to work.