-
Notifications
You must be signed in to change notification settings - Fork 58
Added --namespace flag to k3kcli policy create
#564
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #564 +/- ##
==========================================
- Coverage 59.78% 59.42% -0.36%
==========================================
Files 56 56
Lines 5217 5255 +38
==========================================
+ Hits 3119 3123 +4
- Misses 1811 1842 +31
- Partials 287 290 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
In case the namespace already has a policy attached. we should display a warning and not change the policy. |
| if !apierrors.IsNotFound(err) { | ||
| return 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.
Should we check if the policy was ever bound to one more namespaces and clear it when deleted?
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 discussed offline this should probably responsibility of the controller itself, I've opened an issue about this: #563
pmatseykanets
left a comment
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.
Seems like tests need some finessing due to message format changes.
cli/cmds/cluster_create.go
Outdated
| } | ||
|
|
||
| logrus.Infof("Creating cluster [%s] in namespace [%s]", name, namespace) | ||
| logrus.Infof("Creating cluster %q in namespace %q", name, namespace) |
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.
May not be a good idea to use double quotes with a structured logger. IMHO it impairs readibility.
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.
Yep, the tests fails because when the cli is not running in a TTY it will output the logs in a structured format, while on the console it looks fine.
I'll move this to single quotes!
45992e4 to
4ba97fa
Compare
4ba97fa to
01791be
Compare
Fix #379
The
--namespaceflag was inherited by the global flags, but it was not used in thepolicy createcommand.This PR change this behaviour, adding the option to provide multiple
--namespaceflags during the policy creation to actually bind it to existing namespaces:As suggested also a
--overwriteflag was added. This is used to eventually overwrite existing policy. Without it we are not going to unbind existing policies, but just warn: