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

add openpgp encryption middleware #18

Merged
merged 1 commit into from
Jan 27, 2023
Merged

Conversation

dhia-gharsallaoui
Copy link
Contributor

No description provided.

@wneessen
Copy link
Owner

Thanks for the PR Dhia! A few notes:

  • Could you please write test coverage for the PR?
  • I am wondering if it would make sense to offer different types of actions: encryption-only, signature-only, encrpyt and sign?
  • The logs should go to os.Stderr in my opinion. Does your loggigng library offer this or would it make sense to just use stdlib log? Is JSON logging really preferable for this?

@dhia-gharsallaoui
Copy link
Contributor Author

Thank your for your fast interaction!

  • Could you please write test coverage for the PR?

I will work on it soon when I have free time.

  • I am wondering if it would make sense to offer different types of actions: encryption-only, signature-only, encrpyt and sign?

I don't know as I didn't seen use cases that need only one.

  • The logs should go to os.Stderr in my opinion. Does your loggigng library offer this or would it make sense to just use stdlib log? Is JSON logging really preferable for this?

the Middleware use the log interface so you can inject your logger either JSON or anything Else. In case you didn't inject one he will use the JSON logger as default wich use os.Stderr dependening on the verbosity.

@wneessen
Copy link
Owner

  • Could you please write test coverage for the PR?
    I will work on it soon when I have free time.

Great, thanks!

  • I am wondering if it would make sense to offer different types of actions: encryption-only, signature-only, encrpyt and sign?
    I don't know as I didn't seen use cases that need only one.

Understood. Then let's start with encryption first and we can later add mail signing.

  • The logs should go to os.Stderr in my opinion. Does your loggigng library offer this or would it make sense to just use stdlib log? Is JSON logging really preferable for this?
    the Middleware use the log interface so you can inject your logger either JSON or anything Else. In case you didn't inject one he will use the JSON logger as default wich use os.Stderr dependening on the verbosity.

That makes sense.

@wneessen
Copy link
Owner

Hi @dhia-gharsallaoui, I'm gonna merge this for now, even with the missing tests. I'd like to contribute to this package. This way we can both work on the code. If I find some time, I'll add some tests as well, if that's ok with you.

@wneessen wneessen merged commit 4525e92 into wneessen:main Jan 27, 2023
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.

2 participants