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

fix(restore): consider the banned namespaces while bumping (#7839) #8559

Merged
merged 1 commit into from
Jan 25, 2023

Conversation

mangalaman93
Copy link
Member

We were not considering the banned namespaces while bumping the lease. So, if we restore a backup with namespace 0,1,2 not-banned and 3,4 banned. We will bump only up to 2. This would error out when creating a namespace

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@mangalaman93 mangalaman93 added the slash-to-main PRs which bring slash branch on par with main. label Jan 4, 2023
@mangalaman93 mangalaman93 force-pushed the anurag/cherry-pick-drop-data branch from c06c19d to e08c643 Compare January 4, 2023 05:13
@mangalaman93 mangalaman93 force-pushed the anurag/cherry-pick-drop-data branch from e08c643 to b942479 Compare January 4, 2023 05:30
matthewmcneely
matthewmcneely previously approved these changes Jan 5, 2023
@MichelDiz
Copy link
Contributor

These cherry-picks for branches confuse me a bit. But maybe it's the safest way to avoid breaking something. Only I wonder if the CI is correct. If it is running the right context. It feels like it wasn't triggered here.

@mangalaman93
Copy link
Member Author

Don't worry about that Michel, when we merge, we will merge to main. Feel free to review the change.

skrdgraph
skrdgraph previously approved these changes Jan 6, 2023
@mangalaman93 mangalaman93 added this to the v23.0.0 milestone Jan 11, 2023
@mangalaman93 mangalaman93 force-pushed the anurag/cherry-pick-drop-data branch from e66d8ec to 69dac5f Compare January 17, 2023 07:55
@mangalaman93 mangalaman93 force-pushed the anurag/cherry-pick-drop-data branch from 69dac5f to 6008c41 Compare January 18, 2023 20:23
@mangalaman93 mangalaman93 force-pushed the anurag/cherry-pick-drop-data branch from 6008c41 to 4e1118c Compare January 20, 2023 07:58
@mangalaman93 mangalaman93 force-pushed the anurag/cherry-pick-drop-data branch from 4e1118c to 6c16b0b Compare January 23, 2023 16:09
Base automatically changed from anurag/cherry-pick-drop-data to main January 24, 2023 10:12
@mangalaman93 mangalaman93 dismissed stale reviews from skrdgraph and matthewmcneely via bb76697 January 24, 2023 10:12
We were not considering the banned namespaces while bumping the lease.
So, if we restore a backup with namespace 0,1,2 not-banned and 3,4
banned. We will bump only up to 2. This would error out when creating
a namespace.
@all-seeing-code
Copy link
Contributor

We were not considering the banned namespaces while bumping the lease. So, if we restore a backup with namespace 0,1,2 not-banned and 3,4 banned. We will bump only up to 2. This would error out when creating a namespace

  • Can you explain a little bit more?
  • What if we have a scenario where 0,1 -- allowed 2,3 -- banned and 4,5,6 -- allowed ?
  • Probably needs a test.

@all-seeing-code all-seeing-code self-requested a review January 24, 2023 10:18
@mangalaman93
Copy link
Member Author

@anurags92 in your scenario, when we do restore, we will observe the namespace 6 and we will bump up to the correct value for maxNamespace. Siddhesh is working on a test already.

@coveralls
Copy link

Coverage Status

Coverage: 67.085% (+0.1%) from 66.946% when pulling 3049796 on aman/ban into 5bbea2f on main.

@all-seeing-code all-seeing-code merged commit 5fc279c into main Jan 25, 2023
@all-seeing-code all-seeing-code deleted the aman/ban branch January 25, 2023 15:38
all-seeing-code pushed a commit that referenced this pull request Feb 8, 2023
…8559)

We were not considering the banned namespaces while bumping the lease.
So, if we restore a backup with namespace 0,1,2 not-banned and 3,4
banned. We will bump only up to 2. This would error out when creating a
namespace

Co-authored-by: Naman Jain <[email protected]>
all-seeing-code pushed a commit that referenced this pull request Feb 8, 2023
…8559)

We were not considering the banned namespaces while bumping the lease.
So, if we restore a backup with namespace 0,1,2 not-banned and 3,4
banned. We will bump only up to 2. This would error out when creating a
namespace

Co-authored-by: Naman Jain <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
slash-to-main PRs which bring slash branch on par with main.
Development

Successfully merging this pull request may close these issues.

9 participants