-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
backupccl: clean up old backup syntax code #135134
Conversation
It looks like your PR touches SQL parser code but doesn't add or edit parser tests. Please make sure you add or edit parser tests if you edit the parser. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
8a33ea4
to
da83941
Compare
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.
nice! just a couple nits
PrevBackupURIs: prevBackupURIs, | ||
}, nil | ||
} | ||
|
||
defaultStore, err := makeCloudStorage(ctx, plannedBackupDefaultURI, user) | ||
if err != nil { | ||
return ResolvedDestination{}, err |
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.
as a follow up todo, you could clean up some of the error handling and comments below.
b8dd321
to
8bf2518
Compare
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 should have been more clear: i think this pr should refactor all the code you have here, but in a seperate pr with a release note, i would remove that error path + old cluster setting.
d26253e
to
b61e7a8
Compare
The old backup syntax was fully removed in cockroachdb#133610. This commit removes all the old code that ran for the old syntax. Epic: none Release note: none
b61e7a8
to
26184f4
Compare
TFTR! bors r=msbutler |
The old backup syntax was fully removed in #133610. This commit removes all the old code that ran for the old syntax.
Epic: none
Release note: none