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

feat: add support for raw folder permissions #1054

Merged
merged 1 commit into from May 17, 2023

Conversation

weisdd
Copy link
Collaborator

@weisdd weisdd commented May 15, 2023

Added support for raw folder permissions (JSON).
If we ever add support for teams, we can implement something more complex.

@weisdd weisdd requested review from pb82, NissesSenap and HVBE May 15, 2023 21:27
@weisdd weisdd self-assigned this May 15, 2023
Copy link
Collaborator

@NissesSenap NissesSenap left a comment

Choose a reason for hiding this comment

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

LGTM, this implemention looks good to me. But i added a suggestion as well.

permissions := grapi.PermissionItems{}
err = json.Unmarshal([]byte(cr.Spec.Permissions), &permissions)
if err != nil {
return fmt.Errorf("failed to unmarshal spec.permissions: %w", err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We have an issue for dashboards around if we get an error for the specific dashboard it would be nice if we make this visible in the status as well.
Should we do that for folders also?
Might be a little painful since we have to consider multiple instances.
But for a json error like this it should be easy.

@pb82 pb82 merged commit 865b5c5 into master May 17, 2023
9 checks passed
@pb82 pb82 deleted the feat/folder-raw-permissions branch May 17, 2023 12:38
@weisdd
Copy link
Collaborator Author

weisdd commented May 17, 2023

@NissesSenap yeah, I agree with your suggestion, let's add it in one of the next PRs.

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.

None yet

3 participants