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

improve welcome message #680

Merged
merged 3 commits into from
May 19, 2020
Merged

Conversation

MarinPostma
Copy link
Contributor

fixes #663

image

@MarinPostma MarinPostma requested a review from qdequele May 14, 2020 09:04
meilisearch-http/src/main.rs Outdated Show resolved Hide resolved
meilisearch-http/src/main.rs Outdated Show resolved Hide resolved
Copy link
Member

@Kerollmops Kerollmops left a comment

Choose a reason for hiding this comment

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

Can you also replace all the println by eprintlns, please! Those are debug informations.

@MarinPostma MarinPostma requested a review from Kerollmops May 14, 2020 09:57
Copy link
Member

@Kerollmops Kerollmops left a comment

Choose a reason for hiding this comment

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

Seems great, can you rebase, this way I can merge, please?

@MarinPostma MarinPostma force-pushed the better-welcome branch 3 times, most recently from 735661f to a69fdd9 Compare May 17, 2020 20:48
Copy link
Member

@qdequele qdequele left a comment

Choose a reason for hiding this comment

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

LGTM

info!("If you want to support us or help us; Please consult our Github repo: http://github.com/meilisearch/meilisearch");
info!("If you want to contact us; Please chat with us on http://meilisearch.com or by email to [email protected]");
eprintln!();
eprintln!("Documentation:\t\thttp://docs.meilisearch.com");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
eprintln!("Documentation:\t\thttp://docs.meilisearch.com");
eprintln!("Documentation:\t\thttps://docs.meilisearch.com");

info!("If you want to contact us; Please chat with us on http://meilisearch.com or by email to [email protected]");
eprintln!();
eprintln!("Documentation:\t\thttp://docs.meilisearch.com");
eprintln!("Source code:\t\thttp://github.com/meilisearch/meilisearch");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
eprintln!("Source code:\t\thttp://github.com/meilisearch/meilisearch");
eprintln!("Source code:\t\thttps://github.com/meilisearch/MeiliSearch");

env!("CARGO_PKG_VERSION").to_string()
);

eprintln!();

if let Some(master_key) = &data.api_keys.master {
Copy link
Member

Choose a reason for hiding this comment

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

This might not be pertinent in this PR, but isn't it unsafe to put the Master key (and maybe private/public keys) in the logs?

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
Member

Choose a reason for hiding this comment

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

Not sure why @qdequele have done this, but yeah it can be unsafe if any user have access to the logs of the running server.

Copy link
Member

@eskombro eskombro May 18, 2020

Choose a reason for hiding this comment

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

Should I create an issue on this? (separate issue)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can fix this here, seems related enough

Copy link
Member

Choose a reason for hiding this comment

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

I tried to create a suggestion, but it gets messy as it is done over a mix of removed and added lines. The main idea is just to tell the user that a Master key was set, and that he needs authentication for requests, but deleting all the logs with sensitive keys inside ;)

if let Some(master_key) = &data.api_keys.master {
info!("Master Key: {:?}", master_key);
eprintln!("Master Key:\t{:?}", master_key);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
eprintln!("Master Key:\t{:?}", master_key);
eprintln!("A Master Key has been set. Requests to MeiliSearch won't be authorized unless you provide an authentication key.");

if let Some(master_key) = &data.api_keys.master {
info!("Master Key: {:?}", master_key);
eprintln!("Master Key:\t{:?}", master_key);

if let Some(private_key) = &data.api_keys.private {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if let Some(private_key) = &data.api_keys.private {


if let Some(private_key) = &data.api_keys.private {
info!("Private Key: {:?}", private_key);
eprintln!("Private Key:\t{:?}", private_key);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
eprintln!("Private Key:\t{:?}", private_key);

Comment on lines 118 to 120
}

if let Some(public_key) = &data.api_keys.public {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
}
if let Some(public_key) = &data.api_keys.public {

}

if let Some(public_key) = &data.api_keys.public {
info!("Public Key: {:?}", public_key);
eprintln!("Public Key:\t{:?}", public_key);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
eprintln!("Public Key:\t{:?}", public_key);

}

if let Some(public_key) = &data.api_keys.public {
info!("Public Key: {:?}", public_key);
eprintln!("Public Key:\t{:?}", public_key);
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
}

@MarinPostma
Copy link
Contributor Author

Hold on, that's breaking! if you set the master key, private and public are generated automatically, and printed on startup... @qdequele what do we do?

@qdequele
Copy link
Member

I think you can hide keys on logs. It's not necessary. For me, It's not really breaking because that is just comments and not an interactive functionality. It's the same as changing the command line parameter descriptions. What do you think?

@MarinPostma
Copy link
Contributor Author

How is the user supposed to retrieve the private and public keys that are set by default?

@curquiza
Copy link
Member

With the /keys route, no?

@MarinPostma
Copy link
Contributor Author

all good

@Kerollmops
Copy link
Member

Can you rebase, please? I merge just after.

@MarinPostma
Copy link
Contributor Author

done

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.

Improvements for terminal greeting
5 participants