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

Bugfix/2307 change error message in lakefs setup #2435

Merged
merged 5 commits into from
Sep 12, 2021

Conversation

lynnro314
Copy link
Contributor

@lynnro314 lynnro314 commented Sep 1, 2021

Close: #2307

When trying to set up lakeFS more than once, the error message was not descriptive.
I made two changes:

  1. In the setup phase I added a verification of whether the setup already happened. In this case, a descriptive error message will be printed.
  2. I changed the error message in the UI to be descriptive.

@CLAassistant
Copy link

CLAassistant commented Sep 1, 2021

CLA assistant check
All committers have signed the CLA.

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.

Looks good, just think we need to use the same terminology - setup and not initialized and drop the lakeFS.

Copy link
Contributor

@talSofer talSofer left a comment

Choose a reason for hiding this comment

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

Nice!

Please edit the following to the pr description:

  1. Fixes Non-descriptive error message while trying to create an Admin user more than once  #2307 (this automatically closes the issue when you merge the pr)
  2. fix description. What was the problem, and what solution you implemented.

@talSofer
Copy link
Contributor

talSofer commented Sep 2, 2021

Looks good, just think we need to use the same terminology - setup and not initialized and drop the lakeFS.

Agree with @nopcoder . To understand if setup already happened you are checking whether the metadata manager is already initialized which is an implementation detail. as a user I would want the software to speak the language of the actions I'm taking.

@lynnro314 lynnro314 added this to In progress in lakeFS via automation Sep 2, 2021
@lynnro314 lynnro314 self-assigned this Sep 2, 2021
@ozkatz ozkatz moved this from In progress to Review in progress in lakeFS Sep 2, 2021
os.Exit(1)
}
if initialized {
fmt.Printf("Setup is already complete.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Add new-line or use fmt.Println

@lynnro314 lynnro314 merged commit 56ccefd into master Sep 12, 2021
lakeFS automation moved this from Review in progress to Done Sep 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
lakeFS
  
Done
Development

Successfully merging this pull request may close these issues.

Non-descriptive error message while trying to create an Admin user more than once
4 participants