Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Check the parts where there are NOTES in this doc file. It would be good to keep standards with notes, like below:
A note with capitalized letters starting with a > symbol and bold.
Example:
Also maybe a reword like the following:
if the specified backup is a
postgres
backup and has dependent incremental backups, this backup and all their following dependents will be deleted. This happens because a backup without its parent makes the backup chain invalid and a further recovery of any of the dependent backups would not be possible.This only applies to backups with backup_method="postgres".
My thought is that we should be less wordy and try to use the same words like dependent OR descendant (not both). Also i think that part (i.e., backups taken with
--incremental <backup_to_delete>
) is not so important and might be confusing.The reword is just a suggestion, you do not need to follow it!
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 agree, tbh i don't like this part either "(i.e., backups taken with --incremental <backup_to_delete>)" hahahaha.
I have two points about this:
1 - Do you think the term "postgres backup" is well-understood by the users? I mean, at the end, everything is a postgres backup, right? I've been avoiding this as I think it is mostly used by us to differentiate it from rsync backups but i'm not sure if it is the correct term.
2 - I have the impression that notes are mostly used when you're explaining something "extra", like something that didn't fit in the context but that's still important to mention. In this case, i think explaining this behavior is just a continuation of the existing explanation. What do you think?
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.
1 - good point! If this is misleading to users, we might need to use backup_method="postgres" instead of just "postgres backup". Something like this!
2 - Yeah, i guess i saw the Note in the beginning of the sentence and i thought it would be better to put as a big NOTE. But i guess that explanation could be an IMPORTANT flag or NOTE because in fact it is some kind of note or important information.
But of course, we can wait for more suggestion from the others!
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.
Sure, I'll try a better version later today and ping you here to see what you think.
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.
when adding a NOTE or an IMPORTANT section to the doc, use
or
Not commenting on the content yet as seems that some changes are going to happen @gustabowill
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.
@andremagui @gcalacoci changed it and added it as important note. I'm using the term "block-level incremental backups" instead of "Postgres incremental backups" as I think is clearer and is also what I'm using to explain the
--incremental
option in the other doc PR.