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

backup: fix alter backup schedule failing on uri with spaces #137948

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kev-cao
Copy link
Contributor

@kev-cao kev-cao commented Dec 23, 2024

ALTER BACKUP SCHEDULE being run on a backup schedule whose URI contains spaces would fail with error ERROR: parse "'gs://cockroachdb-backup-testing/kevin spacey?AUTH=implicit'": first path segment in URL cannot contain colon.

This was caused by the fact that despite the FmtBareStrings flag being used, the space character is considered one of the "special characters" outlined in its doc that causes the string to remain wrapped with quotes. Passing this quoted string to url.Parse fails.

Fixes: #134861

Release note (bug fix): ALTER BACKUP SCHEDULE no longer fails on schedules whose collection URI contains a space.

@kev-cao kev-cao requested a review from a team as a code owner December 23, 2024 20:40
@kev-cao kev-cao requested review from msbutler and removed request for a team December 23, 2024 20:40
Copy link

blathers-crl bot commented Dec 23, 2024

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@kev-cao kev-cao requested a review from alicia-l2 December 23, 2024 20:41
@kev-cao kev-cao force-pushed the backup/alter-backups-spaces branch from e04a4db to b90e28c Compare December 23, 2024 21:42
Copy link
Collaborator

@msbutler msbutler left a comment

Choose a reason for hiding this comment

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

Do we need to change how processInto parses uris too?

@kev-cao
Copy link
Contributor Author

kev-cao commented Dec 26, 2024

@msbutler Nope, it does not need to. That being said, I'll update the tests so that it also includes it

`ALTER BACKUP SCHEDULE` being run on a backup schedule whose URI
contains spaces would fail with error `ERROR: parse
"'gs://cockroachdb-backup-testing/kevin spacey?AUTH=implicit'": first
path segment in URL cannot contain colon`.

This was caused by the fact that despite the `FmtBareStrings` flag being
used, the space character is considered one of the "special characters"
outlined in its doc that causes the string to remain wrapped with
quotes. Passing this quoted string to `url.Parse` fails.

Fixes: cockroachdb#134861

Release note (bug fix): `ALTER BACKUP SCHEDULE` no longer fails on
schedules whose collection URI contains a space.
@kev-cao kev-cao force-pushed the backup/alter-backups-spaces branch from b90e28c to bbac419 Compare December 26, 2024 18:15
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.

Backup uri with spaces
3 participants