-
Notifications
You must be signed in to change notification settings - Fork 365
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: [M3-6759] - Handle VPC Account Grants / Permissions / Capabilities #9585
Conversation
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.
@coliu-akamai you can get a read-only VPC by creating a restricted user (if you don't have one already), and then from the main account, going to Users & Grants
> User Permissions
for the restricted user and marking that VPC as read-only for them. Then log in as the restricted user and confirm they can't edit anything for that VPC
packages/manager/.changeset/pr-9585-upcoming-features-1692736112038.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Dajahi Wiley <114682940+dwiley-akamai@users.noreply.github.com>
…12038.md Co-authored-by: Dajahi Wiley <114682940+dwiley-akamai@users.noreply.github.com>
thanks @dwiley-akamai, that worked! |
0175be1
to
9854434
Compare
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.
Looks good so far -- we'll want to have the isFeatureEnabled()
logic in MainContent.tsx
, AddNewMenu.tsx
, and PrimaryNav.tsx
too.
Also, VPCs show up on the landing page even if a restricted user's VPC permissions are set to "None" -- I think that may be an API bug since I tried from the command line and observed a similar result
thanks @dwiley-akamai, just added for those files! Will also make sure to stay up to date on your post about the potential bug |
Co-authored-by: Dajahi Wiley <114682940+dwiley-akamai@users.noreply.github.com>
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.
LGTM, just have to merge in the latest develop
and resolve the conflicts 🚢
48bec66
to
b6f61a4
Compare
3eca80d
to
b6f61a4
Compare
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.
Didn't realize my review got dismissed earlier in the week 😅
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.
Hmm, same thing happened on the Subnets PR with this file being pulled in somehow 🤔
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.
Wondering if it's a prettier or eslint thing? Gonna try to get rid of the change
edit: yeah prettier moves the line down, then eslint doesn't let me commit without changes. I could add --no-verify to bypass, but since prettier will probably keep on moving the line back down anyway, I think it's fine to keep the changes
@dwiley-akamai sorry that was actually me -- I'd rerequested a review for the grant related changes I'd made for the VPC column in the linodes landing table, but since we undid that change, the review request turned out to be unnecessary 😅 |
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.
LGTM other than the "None" issue that Dajahi mentioned!
@hana-linode ty Hana! VPCs with permissions set to none will still show up due to API design, so we're treating them as read_only and have added in explanatory comment for this. gonna merge this! 🎉 |
…ies (linode#9585) * add vpc grant and permissions * yarn changeset * Update packages/api-v4/.changeset/pr-9585-changed-1692736077405.md Co-authored-by: Dajahi Wiley <114682940+dwiley-akamai@users.noreply.github.com> * Update packages/manager/.changeset/pr-9585-upcoming-features-1692736112038.md Co-authored-by: Dajahi Wiley <114682940+dwiley-akamai@users.noreply.github.com> * fix unresolved merge conflict issues * update vpc global grant text * add missing files * Update packages/manager/src/features/VPC/VPCLanding/VPCEditDrawer.tsx Co-authored-by: Dajahi Wiley <114682940+dwiley-akamai@users.noreply.github.com> * add comment to explain VPC permissions --------- Co-authored-by: Dajahi Wiley <114682940+dwiley-akamai@users.noreply.github.com>
…ies (#9585) * add vpc grant and permissions * yarn changeset * Update packages/api-v4/.changeset/pr-9585-changed-1692736077405.md Co-authored-by: Dajahi Wiley <114682940+dwiley-akamai@users.noreply.github.com> * Update packages/manager/.changeset/pr-9585-upcoming-features-1692736112038.md Co-authored-by: Dajahi Wiley <114682940+dwiley-akamai@users.noreply.github.com> * fix unresolved merge conflict issues * update vpc global grant text * add missing files * Update packages/manager/src/features/VPC/VPCLanding/VPCEditDrawer.tsx Co-authored-by: Dajahi Wiley <114682940+dwiley-akamai@users.noreply.github.com> * add comment to explain VPC permissions --------- Co-authored-by: Dajahi Wiley <114682940+dwiley-akamai@users.noreply.github.com>
Description 📝
Add VPC grants, permissions, and capabilities to determine VPC usage
Major Changes 🔄
Preview 📷
User Permissions:
no permissions:
How to test 🧪