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

[enhancement] now refuses to create multiple virtual clusters in same ns #2052

Merged
merged 2 commits into from
Aug 12, 2024

Conversation

facchettos
Copy link
Contributor

What issue type does this pull request address? (keep at least one, remove the others)
/kind enhancement

What does this pull request do? Which issues does it resolve? (use resolves #<issue_number> if possible)
resolves eng-4308

Please provide a short message that should be published in the vcluster release notes
Fixed an issue where vcluster would let users create multiple virtual clusters in the same ns

What else do we need to know?

Copy link

netlify bot commented Aug 9, 2024

Deploy Preview for vcluster-docs canceled.

Built without sensitive environment variables

Name Link
🔨 Latest commit e4f114b
🔍 Latest deploy log https://app.netlify.com/sites/vcluster-docs/deploys/66b9c1362e1e1b00084157ff

@facchettos facchettos force-pushed the refuse-multiple-vcluster-in-ns branch from 30fe6c3 to 78e2396 Compare August 9, 2024 11:37
@facchettos facchettos marked this pull request as ready for review August 9, 2024 11:37
@facchettos facchettos force-pushed the refuse-multiple-vcluster-in-ns branch from 78e2396 to 8b683e1 Compare August 9, 2024 12:59
pkg/cli/create_helm.go Outdated Show resolved Hide resolved
@facchettos facchettos force-pushed the refuse-multiple-vcluster-in-ns branch from 8b683e1 to 6d13723 Compare August 9, 2024 13:08
if v.Namespace == cmd.Namespace && v.Name != args[0] {
return fmt.Errorf("there is already a virtual cluster in namespace %s", cmd.Namespace)
}
}
Copy link
Member

@FabianKramm FabianKramm Aug 12, 2024

Choose a reason for hiding this comment

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

I think this should go into the helm driver only, as there can be many vClusters with the same namespace in the platform in different host clusters

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had it there before, but do we support creating multiple virtual clusters in the same ns with the platform?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it seems that the ListOSS function should work with platform created virtual clusters too, since it's fetching the statefulset/deployment

Copy link
Member

@FabianKramm FabianKramm left a comment

Choose a reason for hiding this comment

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

See comment above

@facchettos facchettos force-pushed the refuse-multiple-vcluster-in-ns branch from 8a452e9 to 6d13723 Compare August 12, 2024 07:47
pkg/cli/create_helm.go Outdated Show resolved Hide resolved
@FabianKramm FabianKramm merged commit 1cbb7c6 into loft-sh:main Aug 12, 2024
61 checks passed
@facchettos facchettos added the backport-to-v0.20 backport this PR to v0.20 branch label Aug 13, 2024
loft-bot pushed a commit that referenced this pull request Aug 13, 2024
… ns (#2052)

* [enhancement] now refuses to create multiple virtual clusters in same ns

* added command context

(cherry picked from commit 1cbb7c6)
@loft-bot
Copy link

💚 All backports created successfully

Status Branch Result
v0.20

Questions ?

Please refer to the Backport tool documentation and see the Github Action logs for details

FabianKramm added a commit that referenced this pull request Aug 13, 2024
[v0.20] [enhancement] now refuses to create multiple virtual clusters in same ns (#2052)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-to-v0.20 backport this PR to v0.20 branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants