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

Document email usage in the api #6717

Merged
merged 3 commits into from
Oct 9, 2023
Merged

Document email usage in the api #6717

merged 3 commits into from
Oct 9, 2023

Conversation

itaiad200
Copy link
Contributor

closes #6684

@itaiad200 itaiad200 added the v1.0.0-blocker Issues that should be closed before going out with v1.0.0 label Oct 5, 2023
@itaiad200 itaiad200 marked this pull request as ready for review October 5, 2023 22:21
@github-actions
Copy link

github-actions bot commented Oct 5, 2023

♻️ PR Preview 519214e has been successfully destroyed since this PR has been closed.

🤖 By surge-preview

@itaiad200 itaiad200 added the exclude-changelog PR description should not be included in next release changelog label Oct 6, 2023
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.

This seems great and is an obvious improvement, a tonne of thanks for that! Not approving as I lack the definitive knowledge required to say it's correct.

email:
type: string
description: |
The email address of the user. If API authentication is enabled, this field is mandatory and will be invited to login.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it unique? (I assume not, but it pays to be explicit)
Is it case sensitive?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

lakeFS doesn't have such restrictions, in most cases we connect with an external auth provider which does cause the email to be unique

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added an explanation on APIAuthenticator enforcements.

api/swagger.yml Outdated Show resolved Hide resolved
@@ -672,15 +672,21 @@ components:
properties:
id:
Copy link
Contributor

Choose a reason for hiding this comment

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

We should say how to present the user. I think it's display the first non-blank of friendly name, email, id. But I'm honestly not sure.
(Also now that anything except the last is non-unique! So probably good to display to the user themselves, but not so good to display in a list of users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@guy-har @Isan-Rivkin I need help answering this as the main consumers of this is us (Cloud, Enterprise)

Copy link
Contributor

Choose a reason for hiding this comment

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

Currently, the behavior in the UI isn't consistent
when listing users we display the first non-blank of email, id
when showing the current user in the navbar we display the first non-blank of friendly name, id

IMO we should align them both to the first non-blank of friendly name, email, and id but still provide a way to see the ID.
Having that said, is this something that should be documented or just a client decision?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. Since we don't have any strict representation of the users, I would refrain from adding one to the API documentation. Once we'll have more alignment this can be revised. But until then - less is more.

Copy link
Contributor

@guy-har guy-har left a comment

Choose a reason for hiding this comment

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

LGTM

@itaiad200 itaiad200 merged commit eb62d5d into master Oct 9, 2023
32 of 33 checks passed
@itaiad200 itaiad200 deleted the 6684-document-email branch October 9, 2023 11:16
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 v1.0.0-blocker Issues that should be closed before going out with v1.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document email on users
4 participants