fix: rbac validate command can now take either namespace or policy-file #15543

ashinsabu3 · 2023-09-18T13:37:52Z

fixes: #6126
This fix now allows the command to take only a namespace flag if required.

Checklist:

codecov · 2023-09-18T15:42:04Z

Codecov Report

Attention: 17 lines in your changes are missing coverage. Please review.

Comparison is base (ab1cc50) 49.54% compared to head (1659823) 49.68%.
Report is 4 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #15543      +/-   ##
==========================================
+ Coverage   49.54%   49.68%   +0.14%     
==========================================
  Files         269      269              
  Lines       46583    46629      +46     
==========================================
+ Hits        23079    23169      +90     
+ Misses      21232    21186      -46     
- Partials     2272     2274       +2

Files	Coverage Δ
cmd/argocd/commands/admin/settings_rbac.go	`51.43% <56.41%> (+24.79%)`	⬆️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

crenshaw-dev · 2023-09-18T16:40:47Z

@ashinsabu3 can you write a test to prevent regressions?

ashinsabu3 · 2023-09-18T17:25:33Z

@ashinsabu3 can you write a test to prevent regressions?

@crenshaw-dev I don't quite understand what you mean by this. If you mean a unit test, I'm not exactly sure how I would test the Rbac command functions.

crenshaw-dev · 2023-09-18T18:48:50Z

Yep, unit testing! Cobra has unit testing utilities. I think there are a couple examples using it in our code base. It'll probably require a bit of mocking to get working.

ashinsabu3 · 2023-09-19T20:36:41Z

I've added a test to check the command being returned, but couldn't get the test for it's execution working. The more I went into trying to get it working, it felt less like a unit test. I've commented it out for now @crenshaw-dev

…le as arg Signed-off-by: Ashin Sabu <ashin.sabu@harness.io>

Signed-off-by: Ashin Sabu <ashin.sabu@harness.io>

ashinsabu3 · 2023-09-30T15:23:46Z

The previous CI failures seem to be due to some flaky e2e tests, not sure, all passed when run on a weekend night 🙂

Signed-off-by: Ashin Sabu <ashin.sabu@harness.io>

cmd/argocd/commands/admin/settings_rbac.go

cmd/argocd/commands/admin/settings_rbac_test.go

ishitasequeira · 2023-10-17T12:35:40Z

cmd/argocd/commands/admin/settings_rbac.go

 	var (
 		policyFile string
 	)

 	var command = &cobra.Command{
-		Use:   "validate --policy-file=POLICYFILE",
+		Use:   "validate --policy-file POLICYFILE [--namespace NAMESPACE]",


As policy-file is also optional now, put it in square brackets.

Suggested change

Use: "validate --policy-file POLICYFILE [--namespace NAMESPACE]",

Use: "validate [--policy-file POLICYFILE] [--namespace NAMESPACE]",

ishitasequeira · 2023-10-17T12:36:25Z

cmd/argocd/commands/admin/settings_rbac.go

 		Short: "Validate RBAC policy",
 		Long: `
 Validates an RBAC policy for being syntactically correct. The policy must be
-a local file, and in either CSV or K8s ConfigMap format.
+a local file or a K8s ConfigMap in the provided namespace, and in either CSV or K8s ConfigMap format.


While we are updating the command, can we add an example as well?

Adding one, let me know if the wording looks fine

The wording looks good! Thanks!

ishitasequeira · 2023-10-17T12:37:39Z

cmd/argocd/commands/admin/settings_rbac.go

 	return command
 }

 // NewRBACCanRoleCommand is the command for 'rbac can-role'
-func NewRBACCanCommand() *cobra.Command {
+func NewRBACCanCommand(clientConfig clientcmd.ClientConfig) *cobra.Command {


Any reason to put the clientConfig as a parameter? If we are taking namespace as an input, do we need to fetch the namespace from clientConfig rather than adding namespace as a variable like policyFile?

I've modified this back, at the time I missed something (most likely the issue you commented below
the flag &policyFile is repeated. This should be &namespace? ) and while testing the command it repeatedly returned some segmentation fault so I thought passing down the clientConfig param would fix it.

I've changed it back now that I know why my command was failing at the time.

cmd/argocd/commands/admin/settings_rbac.go

Signed-off-by: Ashin Sabu <ashin.sabu@harness.io>

ashinsabu3 · 2023-10-24T17:33:45Z

@ishitasequeira Addressed comments you had. Do the changes look good?

ishitasequeira

Thanks @ashinsabu3! Changes look good!

ishitasequeira · 2023-10-26T03:31:41Z

cmd/argocd/commands/admin/settings_rbac.go

 		Short: "Validate RBAC policy",
 		Long: `
 Validates an RBAC policy for being syntactically correct. The policy must be
-a local file, and in either CSV or K8s ConfigMap format.
+a local file or a K8s ConfigMap in the provided namespace, and in either CSV or K8s ConfigMap format.


The wording looks good! Thanks!

…le (argoproj#15543) * fix: rbac validate command can now take either namespace or policy-file as arg Signed-off-by: Ashin Sabu <ashin.sabu@harness.io> * remove changes to generated text Signed-off-by: Ashin Sabu <ashin.sabu@harness.io> * unit test for rbacvalidatecommand Signed-off-by: Ashin Sabu <ashin.sabu@harness.io> * codegen changes Signed-off-by: Ashin Sabu <ashin.sabu@harness.io> * retrigger ci pipeline Signed-off-by: Ashin Sabu <ashin.sabu@harness.io> * retrigger ci pipeline Signed-off-by: Ashin Sabu <ashin.sabu@harness.io> * review comments and test changes Signed-off-by: Ashin Sabu <ashin.sabu@harness.io> * codegen changes Signed-off-by: Ashin Sabu <ashin.sabu@harness.io> * codegen changes - post rebase Signed-off-by: Ashin Sabu <ashin.sabu@harness.io> --------- Signed-off-by: Ashin Sabu <ashin.sabu@harness.io> Signed-off-by: jmilic1 <70441727+jmilic1@users.noreply.github.com>

…le (argoproj#15543) * fix: rbac validate command can now take either namespace or policy-file as arg Signed-off-by: Ashin Sabu <ashin.sabu@harness.io> * remove changes to generated text Signed-off-by: Ashin Sabu <ashin.sabu@harness.io> * unit test for rbacvalidatecommand Signed-off-by: Ashin Sabu <ashin.sabu@harness.io> * codegen changes Signed-off-by: Ashin Sabu <ashin.sabu@harness.io> * retrigger ci pipeline Signed-off-by: Ashin Sabu <ashin.sabu@harness.io> * retrigger ci pipeline Signed-off-by: Ashin Sabu <ashin.sabu@harness.io> * review comments and test changes Signed-off-by: Ashin Sabu <ashin.sabu@harness.io> * codegen changes Signed-off-by: Ashin Sabu <ashin.sabu@harness.io> * codegen changes - post rebase Signed-off-by: Ashin Sabu <ashin.sabu@harness.io> --------- Signed-off-by: Ashin Sabu <ashin.sabu@harness.io>

…le (argoproj#15543) * fix: rbac validate command can now take either namespace or policy-file as arg Signed-off-by: Ashin Sabu <ashin.sabu@harness.io> * remove changes to generated text Signed-off-by: Ashin Sabu <ashin.sabu@harness.io> * unit test for rbacvalidatecommand Signed-off-by: Ashin Sabu <ashin.sabu@harness.io> * codegen changes Signed-off-by: Ashin Sabu <ashin.sabu@harness.io> * retrigger ci pipeline Signed-off-by: Ashin Sabu <ashin.sabu@harness.io> * retrigger ci pipeline Signed-off-by: Ashin Sabu <ashin.sabu@harness.io> * review comments and test changes Signed-off-by: Ashin Sabu <ashin.sabu@harness.io> * codegen changes Signed-off-by: Ashin Sabu <ashin.sabu@harness.io> * codegen changes - post rebase Signed-off-by: Ashin Sabu <ashin.sabu@harness.io> --------- Signed-off-by: Ashin Sabu <ashin.sabu@harness.io> Signed-off-by: Kevin Lyda <kevin@lyda.ie>

…le (argoproj#15543) * fix: rbac validate command can now take either namespace or policy-file as arg Signed-off-by: Ashin Sabu <ashin.sabu@harness.io> * remove changes to generated text Signed-off-by: Ashin Sabu <ashin.sabu@harness.io> * unit test for rbacvalidatecommand Signed-off-by: Ashin Sabu <ashin.sabu@harness.io> * codegen changes Signed-off-by: Ashin Sabu <ashin.sabu@harness.io> * retrigger ci pipeline Signed-off-by: Ashin Sabu <ashin.sabu@harness.io> * retrigger ci pipeline Signed-off-by: Ashin Sabu <ashin.sabu@harness.io> * review comments and test changes Signed-off-by: Ashin Sabu <ashin.sabu@harness.io> * codegen changes Signed-off-by: Ashin Sabu <ashin.sabu@harness.io> * codegen changes - post rebase Signed-off-by: Ashin Sabu <ashin.sabu@harness.io> --------- Signed-off-by: Ashin Sabu <ashin.sabu@harness.io>

crenshaw-dev changed the title ~~fix: rbac validate command can now take either namespace or policy-fi…~~ fix: rbac validate command can now take either namespace or policy-file Sep 18, 2023

ashinsabu3 requested a review from a team as a code owner September 19, 2023 20:26

ashinsabu3 force-pushed the rbac-validate-fix branch from d622b00 to b05e753 Compare September 19, 2023 20:28

ashinsabu3 requested a review from a team as a code owner September 28, 2023 09:45

ashinsabu3 force-pushed the rbac-validate-fix branch from 74ec46e to 8b05340 Compare September 29, 2023 06:42

ashinsabu3 added 5 commits September 29, 2023 12:14

fix: rbac validate command can now take either namespace or policy-fi…

bf28108

…le as arg Signed-off-by: Ashin Sabu <ashin.sabu@harness.io>

remove changes to generated text

cb8394c

Signed-off-by: Ashin Sabu <ashin.sabu@harness.io>

unit test for rbacvalidatecommand

dfea6d9

Signed-off-by: Ashin Sabu <ashin.sabu@harness.io>

codegen changes

83a2bb2

Signed-off-by: Ashin Sabu <ashin.sabu@harness.io>

retrigger ci pipeline

e96000c

Signed-off-by: Ashin Sabu <ashin.sabu@harness.io>

ashinsabu3 force-pushed the rbac-validate-fix branch from 8b05340 to e96000c Compare September 29, 2023 16:12

ashinsabu3 and others added 3 commits October 4, 2023 17:30

Merge branch 'master' into rbac-validate-fix

5da2048

retrigger ci pipeline

f9fc4a3

Signed-off-by: Ashin Sabu <ashin.sabu@harness.io>

Merge branch 'master' into rbac-validate-fix

447510f

ishitasequeira requested changes Oct 17, 2023

View reviewed changes

ashinsabu3 and others added 4 commits October 19, 2023 17:43

review comments and test changes

64605bd

Signed-off-by: Ashin Sabu <ashin.sabu@harness.io>

codegen changes

69c8341

Signed-off-by: Ashin Sabu <ashin.sabu@harness.io>

Merge branch 'argoproj:master' into rbac-validate-fix

6aeaeef

codegen changes - post rebase

1659823

Signed-off-by: Ashin Sabu <ashin.sabu@harness.io>

ashinsabu3 requested a review from ishitasequeira October 19, 2023 22:13

ishitasequeira approved these changes Oct 26, 2023

View reviewed changes

ishitasequeira merged commit 48f175b into argoproj:master Oct 30, 2023
25 checks passed

ashinsabu3 deleted the rbac-validate-fix branch February 19, 2024 10:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: rbac validate command can now take either namespace or policy-file #15543

fix: rbac validate command can now take either namespace or policy-file #15543

ashinsabu3 commented Sep 18, 2023 •

edited

Loading

codecov bot commented Sep 18, 2023 •

edited

Loading

crenshaw-dev commented Sep 18, 2023

ashinsabu3 commented Sep 18, 2023 •

edited

Loading

crenshaw-dev commented Sep 18, 2023

ashinsabu3 commented Sep 19, 2023 •

edited

Loading

ashinsabu3 commented Sep 30, 2023 •

edited

Loading

ishitasequeira Oct 17, 2023

ashinsabu3 Oct 19, 2023

ishitasequeira Oct 17, 2023

ashinsabu3 Oct 19, 2023

ishitasequeira Oct 26, 2023

ishitasequeira Oct 17, 2023

ashinsabu3 Oct 19, 2023 •

edited

Loading

ashinsabu3 commented Oct 24, 2023

ishitasequeira left a comment

ishitasequeira Oct 26, 2023

	Use: "validate --policy-file POLICYFILE [--namespace NAMESPACE]",
	Use: "validate [--policy-file POLICYFILE] [--namespace NAMESPACE]",

fix: rbac validate command can now take either namespace or policy-file #15543

fix: rbac validate command can now take either namespace or policy-file #15543

Conversation

ashinsabu3 commented Sep 18, 2023 • edited Loading

codecov bot commented Sep 18, 2023 • edited Loading

Codecov Report

crenshaw-dev commented Sep 18, 2023

ashinsabu3 commented Sep 18, 2023 • edited Loading

crenshaw-dev commented Sep 18, 2023

ashinsabu3 commented Sep 19, 2023 • edited Loading

ashinsabu3 commented Sep 30, 2023 • edited Loading

ishitasequeira Oct 17, 2023

Choose a reason for hiding this comment

ashinsabu3 Oct 19, 2023

Choose a reason for hiding this comment

ishitasequeira Oct 17, 2023

Choose a reason for hiding this comment

ashinsabu3 Oct 19, 2023

Choose a reason for hiding this comment

ishitasequeira Oct 26, 2023

Choose a reason for hiding this comment

ishitasequeira Oct 17, 2023

Choose a reason for hiding this comment

ashinsabu3 Oct 19, 2023 • edited Loading

Choose a reason for hiding this comment

ashinsabu3 commented Oct 24, 2023

ishitasequeira left a comment

Choose a reason for hiding this comment

ishitasequeira Oct 26, 2023

Choose a reason for hiding this comment

ashinsabu3 commented Sep 18, 2023 •

edited

Loading

codecov bot commented Sep 18, 2023 •

edited

Loading

ashinsabu3 commented Sep 18, 2023 •

edited

Loading

ashinsabu3 commented Sep 19, 2023 •

edited

Loading

ashinsabu3 commented Sep 30, 2023 •

edited

Loading

ashinsabu3 Oct 19, 2023 •

edited

Loading