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

ACLs: add deprecation banner #8057

Merged
merged 2 commits into from
Aug 6, 2024

Conversation

N-o-Z
Copy link
Member

@N-o-Z N-o-Z commented Aug 5, 2024

Closes #8056

Preliminary signal for our users, expressing the intention to move ACLs

@N-o-Z N-o-Z added the exclude-changelog PR description should not be included in next release changelog label Aug 5, 2024
@N-o-Z N-o-Z self-assigned this Aug 5, 2024
Copy link

github-actions bot commented Aug 5, 2024

E2E Test Results - DynamoDB Local - Local Block Adapter

13 passed

Copy link

github-actions bot commented Aug 5, 2024

E2E Test Results - Quickstart

10 passed

Copy link
Collaborator

@ozkatz ozkatz left a comment

Choose a reason for hiding this comment

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

Thanks - I agree a heads up in the product makes sense! I suggest moving it to the admin tab.

@@ -28,8 +28,10 @@ export const AuthLayout = () => {
<Container fluid="xl">
<Row className="mt-5">
<div>
<Alert variant="warning" title="ACL Deprecation"><InfoIcon/>{" "}<b>ACLs are moving out of core lakeFS!</b>{" "}See our <Alert.Link href={"https://lakefs.io/blog/why-moving-acls-out-of-core-lakefs/"}>announcement</Alert.Link>{" "}
Copy link
Collaborator

Choose a reason for hiding this comment

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

60% of installations are single user anyway, and this is perhaps too intimidating. I suggest:

  1. put it only in the administration tab (dunno if hard to do, but nice to have - only show if len(users) > 1)
  2. change the wording: from "our annuncement" to "the announcement"

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. It's only in the administration tab + done
  2. Done

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry to be contrary. Consider the fact that there is little reason for a user ever to go to the Admin tab. I would expect an admin of a medium-sized lakeFS installation to manage users maybe a few times a year.1 Such an admin would never see the deprecation banner.

Footnotes

  1. Can we estimate whether this is true?

Copy link
Member Author

Choose a reason for hiding this comment

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

Though I understand your point, I think the decision on where to show it is a product one.
@ozkatz, do you still think we should place it only on the administration tab scope?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think so. >60% of users shouldn't care about this change, no reason to instill fear. On the other hand, while I agree with @arielshaqed that the admin page isn't visited frequently, it's still probably a higher frequency than upgrading lakeFS versions.

No perfect solution here..

@N-o-Z N-o-Z requested a review from ozkatz August 6, 2024 00:37
Copy link
Contributor

@arielshaqed arielshaqed left a comment

Choose a reason for hiding this comment

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

Approving as code works. However I believe that this banner will not be visible enough to many users.

Copy link
Collaborator

@ozkatz ozkatz left a comment

Choose a reason for hiding this comment

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

Thanks!

@N-o-Z N-o-Z merged commit 01cc109 into master Aug 6, 2024
37 checks passed
@N-o-Z N-o-Z deleted the task/acl-update-administration-banner-8056 branch August 6, 2024 17:59
@N-o-Z N-o-Z mentioned this pull request Aug 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exclude-changelog PR description should not be included in next release changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ACLs: Update administration banner
3 participants