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

Add cluster role section to a module #668

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

nikonhub
Copy link
Contributor

@nikonhub nikonhub commented Nov 1, 2024

closes #436

πŸ“‘ Description

Add ClusterRole section to module resources

βœ… Checks

  • I have tested my code (provide screenshots or screen recordings of a working solution)
  • I have performed a self-review of my code

β„Ή Additional context

image

@nikonhub nikonhub requested a review from a team as a code owner November 1, 2024 07:34
Copy link
Collaborator

@petar-cvit petar-cvit left a comment

Choose a reason for hiding this comment

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

@nikonhub thanks for the PR, looks awesome! I left one comment on one edge case.

Comment on lines 71 to 75
{apiGroups.map((group) => (
<Tag key={group} color="blue">
{group || "*"}
</Tag>
))}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not all rules will have apiGroups. With cluster roles, you can have a rule that has a nonResourceURLs field, and those rules will not have the apiGroups property.

You can create a Module from the Prometheus bitnami helm chart and you will get one such cluster role which has a single rule without apiGroups.

Because of that, the apiGroups here will be null and calling .map will break the UI

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes right. I've fixed this case.

image

Should we display something for this rule ?

{
	"2": {
		"verbs": [
			"get"
		],
		"nonResourceURLs": [
			"/metrics"
		]
	}
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

It might be a bit confusing if you see a rule without any resources and just the verbs.
Can we conditionally add a column to this table? I was thinking of adding a nonResourceURLs column if there is at least one rule with the property. If not, I would leave those three columns. In that case, I would put the verbs as the first column, followed by APIGroups and resources and, if needed, nonREsourceURLs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@nikonhub, can you check my comment above and add a column for nonResourceURLs if possible

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ClusterRole Backend Implementation
2 participants