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

roachprod: don't overwrite tenant in pgurl expansion #123765

Merged

Conversation

renatolabs
Copy link
Contributor

Previously, the {pgurl} expansion would force the cluster connection parameter to system. This means that if the caller previously configured a different default virtual cluster, pgurl would ignore that and continue connecting to system, which is quite surprising.

In this commit, we only set an explicit cluster connection parameter if the user passed one; this should allow pgurl users to connect to the default tenant by default.

Epic: none

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

Previously, the `{pgurl}` expansion would force the `cluster`
connection parameter to `system`. This means that if the caller
previously configured a different default virtual cluster, `pgurl`
would ignore that and continue connecting to `system`, which is
quite surprising.

In this commit, we only set an explicit `cluster` connection parameter
if the user passed one; this should allow `pgurl` users to connect to
the default tenant by default.

Epic: none

Release note: None
@renatolabs renatolabs force-pushed the rc/roachprod-dont-overwrite-tenant branch from fc1b4fd to 4ea8f39 Compare May 7, 2024 19:51
@renatolabs renatolabs marked this pull request as ready for review May 7, 2024 21:04
@renatolabs renatolabs requested a review from a team as a code owner May 7, 2024 21:04
@renatolabs renatolabs requested review from srosenberg and DarrylWong and removed request for a team May 7, 2024 21:04
@renatolabs
Copy link
Contributor Author

Multi-tenant tests pass on this branch.

Copy link
Contributor

@DarrylWong DarrylWong left a comment

Choose a reason for hiding this comment

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

LGTM!

@renatolabs
Copy link
Contributor Author

TFTRs!

bors r=DarrylWong,srosenberg

@renatolabs renatolabs added backport-23.1.x Flags PRs that need to be backported to 23.1 backport-23.2.x Flags PRs that need to be backported to 23.2. backport-24.1.x Flags PRs that need to be backported to 24.1. labels May 8, 2024
@craig craig bot merged commit a1e3ec3 into cockroachdb:master May 8, 2024
22 checks passed
Copy link

blathers-crl bot commented May 8, 2024

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


error creating merge commit from 4ea8f39 to blathers/backport-release-23.1-123765: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.1.x failed. See errors above.


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

@renatolabs renatolabs deleted the rc/roachprod-dont-overwrite-tenant branch May 9, 2024 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-23.1.x Flags PRs that need to be backported to 23.1 backport-23.2.x Flags PRs that need to be backported to 23.2. backport-24.1.x Flags PRs that need to be backported to 24.1.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants