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

Empty Email check implemented #2969

Merged
merged 5 commits into from
Oct 14, 2024
Merged

Conversation

gagandeepp
Copy link
Contributor

@gagandeepp gagandeepp commented Oct 10, 2024

Fixes: #2949

@@ -137,6 +138,10 @@ func (s *Action) initGenerateIdentity(ctx context.Context, crypto backend.Crypto
}

email, err = termio.AskForString(ctx, "📧 What is your email?", email)

if strings.TrimSpace(email) == "" {
return fmt.Errorf("failed to create a usable key pair")
Copy link

Choose a reason for hiding this comment

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

Disclaimer: I'm not a maintainer. But I think this message could be more user-friendly. Something like: "You did not provide an email. An email is required to generate a gpg key pair, so please provide one."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jcrben Can you please resolve this conversation?

@gagandeepp gagandeepp force-pushed the validate_email branch 2 times, most recently from cc224eb to 58ec9d7 Compare October 11, 2024 08:19
@gagandeepp
Copy link
Contributor Author

@dominikschulz Please review this

@@ -137,6 +138,10 @@ func (s *Action) initGenerateIdentity(ctx context.Context, crypto backend.Crypto
}

email, err = termio.AskForString(ctx, "📧 What is your email?", email)

if strings.TrimSpace(email) == "" {
Copy link
Member

Choose a reason for hiding this comment

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

nit: I think checking for the email string should happen after error handling in the next block, i.e. please move these lines below line 147.

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 Author

Choose a reason for hiding this comment

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

@dominikschulz Can you please resolve this conversation?

Signed-off-by: gagandeepp <[email protected]>
Signed-off-by: gagandeepp <[email protected]>
Signed-off-by: gagandeepp <[email protected]>
Signed-off-by: gagandeepp <[email protected]>
Signed-off-by: gagandeepp <[email protected]>
@jcrben
Copy link

jcrben commented Oct 11, 2024

What's the story on the age backend? Does the bug also happen there? I don't really know much about age, not sure if it requires an email.

@gagandeepp
Copy link
Contributor Author

What's the story on the age backend? Does the bug also happen there? I don't really know much about age, not sure if it requires an email.

@jcrben honestly no idea about it

@gagandeepp
Copy link
Contributor Author

@dominikschulz please review

Copy link
Member

@dominikschulz dominikschulz left a comment

Choose a reason for hiding this comment

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

LGTM

@gagandeepp
Copy link
Contributor Author

@dominikschulz Can you please add hacktoberfest label to the issue?

@dominikschulz dominikschulz merged commit 75baa9b into gopasspw:master Oct 14, 2024
7 of 8 checks passed
@AnomalRoil
Copy link
Member

No need for emails with the age crypto backend, unless one is using the gitfs storage backend with it.

@dominikschulz
Copy link
Member

@AnomalRoil I don't think we have the infrastructure to plumb through these different backend requirements, yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gopass setup should validate empty email
4 participants