8000 fix: Fix logic when returning default roles by ruggi99 · Pull Request #490 · supabase/postgres-meta · GitHub
[go: up one dir, main page]
Skip to content

fix: Fix logic when ret 8000 urning default roles #490

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jan 30, 2023
Merged

fix: Fix logic when returning default roles #490

merged 3 commits into from
Jan 30, 2023

Conversation

ruggi99
Copy link
Contributor
@ruggi99 ruggi99 commented Jan 28, 2023 •
edited
Loading

What kind of change does this PR introduce?

Bug fix.
Fixes logic with includeDefaultRoles. Actually a breaking change.
Maybe I should change includeDefaultRoles to true to not change the behavior?

What is the current behavior?

Please link any relevant issues here.

What is the new behavior?

Feel free to include screenshots if it includes visual changes.

Additional context

Add any other context or screenshots.

@ruggi99 ruggi99 requested a review from a team as a code owner January 28, 2023 09:56
@soedirgo
Copy link
Member

The list of default/predefined roles can be checked here - I don't think we need to expose this in the response.

kevcodez
kevcodez previously requested changes Jan 28, 2023
View reviewed changes
@ruggi99
Copy link
Contributor Author
ruggi99 commented Jan 28, 2023 •
edited
Loading

Mmm yeah, I messed up a bit I think.
My proposed solution should add that field and tell Supabase interface to not edit that role because it's needed in Supabase ecosystem.
So for example, when and if Custom roles will be implemented Supabase interface should know if a role is editable or not.
That's what I'm trying to do.
But I realize that DEFAULT_ROLES is not usable here.
Should I try creating a new constant SUPABASE_ROLES?

@soedirgo
Copy link
Member

No worries! We want to minimize Supabase-isms in pg-meta, so other users can benefit from it too.

tell Supabase interface to not edit that role

By Supabase interface, do you mean the dashboard at https://app.supabase.com? If so, we can do this on the dashboard side. E.g. if you go to the Table Editor and click on the schema dropdown, you'll see that there are "protected schemas":

Screenshot 2023-01-28 at 7 35 59 PM

We can do something similar for user-defined roles - we just dub Supabase internal roles "protected roles".

@ruggi99
Copy link
Contributor Author
ruggi99 commented Jan 28, 2023

@soedirgo yes I meant Supabase Dashboard.
So you are suggesting to hardcode "protected" roles in Supabase Dashboard?
Since it's a config (protected roles can change in time) I thought it would be best to put them in config API or something similar so I chose postgres-meta.
Since you want to keep postgres-meta Supabase agnostic I'll revert the changes and keep the bug fix

@ruggi99 ruggi99 changed the title chore: Add is_default_role when listing roles + bug fix fix: Fix logic when returning default roles Jan 29, 2023
@soedirgo soedirgo merged commit 60d5d70 into supabase:master Jan 30, 2023
@ruggi99 ruggi99 deleted the roles-system-role branch January 30, 2023 08:10
@github-actions
Copy link

🎉 This PR is included in version 0.60.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

avallete pushed a commit that referenced this pull request May 13, 2025
@ruggi99 @soedirgo
fix: logic when returning default roles (#490)
629c33d 
* Bug fix returning default roles

* refactor: replace DEFAULT_ROLES w/ prefix test

* test: includeDefaultRoles

---------

Co-authored-by: Bobbie Soedirgo <bobbie@soedirgo.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevcodez kevcodez kevcodez left review comments

@soedirgo soedirgo soedirgo approved these changes

Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ruggi99 @soedirgo @kevcodez
0