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

make authorization errors readable in the UI, change /config to require re fs permissions since it returns storage config #2056

Merged
merged 5 commits into from
Jun 8, 2021

Conversation

ozkatz
Copy link
Collaborator

@ozkatz ozkatz commented Jun 3, 2021

Closes #2028

@ozkatz ozkatz requested review from nopcoder and guy-har June 3, 2021 12:35
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
I Prefer that another reviewer goes over the SQL migrations before you merge

@ozkatz
Copy link
Collaborator Author

ozkatz commented Jun 3, 2021

LGTM
I Prefer that another reviewer goes over the SQL migrations before you merge

@guy-har for reference, see https://github.com/treeverse/lakeFS/blob/master/pkg/ddl/000024_actions_auth_policies.up.sql - I use the exact same logic with a different policy.

Copy link
Contributor

@nopcoder nopcoder left a comment

Choose a reason for hiding this comment

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

LGTM!

@ozkatz ozkatz force-pushed the fix/2028-uninformative-auth-errors branch from 28ee852 to fa643f3 Compare June 7, 2021 11:46
@ozkatz ozkatz requested a review from arielshaqed June 7, 2021 11:46
@ozkatz
Copy link
Collaborator Author

ozkatz commented Jun 7, 2021

@arielshaqed see breaking change (/config to /config/storage)

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.

Cool; thanks! I shall update #2002 to use getStorageConfig and compute the warnings locally.

api/swagger.yml Outdated
operationId: getConfig
description: retrieve the lakefs config
operationId: getStorageConfig
description: retrieve the lakefs storage configuration
Copy link
Contributor

Choose a reason for hiding this comment

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

Drop the "the", just "lakeFS config" -- it's cleaner.

Suggested change
description: retrieve the lakefs storage configuration
description: retrieve lakeFS storage configuration

(and elsewhere).

--log-format string set logging output format
--log-level string set logging level (default "none")
--log-output string set logging output file
--no-color don't use fancy output colors (default when not attached to an interactive terminal)
Copy link
Contributor

Choose a reason for hiding this comment

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

These all seem unrelated to this PR.

@arielshaqed
Copy link
Contributor

@ozkatz not pulling this because some validation failed. Instead merging #2002 into this one.

@ozkatz ozkatz merged commit 39033d0 into master Jun 8, 2021
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.

(UI) Uninformative errors
4 participants