-
Notifications
You must be signed in to change notification settings - Fork 9
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
feat: [CO 621] Allow LE certificates to be generated from Mailbox endpoint #175
Conversation
store/src/main/java/com/zimbra/cs/service/admin/CertificateNotificationManager.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the whole map logic is a bit confusing. Maybe consider a more streamlined version where you just do, from the notify:
- parse output
- create global mime message
- create domain mime message
- send both messages using the same sender
This way we avoid having if conditions in the same method to handle domain and global case.
it is also easier to spot the wanted behavior.
store/src/main/java/com/zimbra/cs/service/admin/CertificateNotificationManager.java
Outdated
Show resolved
Hide resolved
store/src/main/java/com/zimbra/cs/service/admin/CertificateNotificationManager.java
Outdated
Show resolved
Hide resolved
store/src/main/java/com/zimbra/cs/service/admin/CertificateNotificationManager.java
Outdated
Show resolved
Hide resolved
store/src/main/java/com/zimbra/cs/service/admin/CertificateNotificationManager.java
Outdated
Show resolved
Hide resolved
store/src/main/java/com/zimbra/cs/service/admin/CertificateNotificationManager.java
Outdated
Show resolved
Hide resolved
store/src/main/java/com/zimbra/cs/service/admin/CertificateNotificationManager.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added some suggestions and requested some changes.
store/src/main/java/com/zimbra/cs/service/admin/CertificateNotificationManager.java
Outdated
Show resolved
Hide resolved
store/src/main/java/com/zimbra/cs/service/admin/CertificateNotificationManager.java
Outdated
Show resolved
Hide resolved
store/src/main/java/com/zimbra/cs/service/admin/CertificateNotificationManager.java
Show resolved
Hide resolved
store/src/main/java/com/zimbra/cs/service/admin/CertificateNotificationManager.java
Outdated
Show resolved
Hide resolved
store/src/main/java/com/zimbra/cs/service/admin/CertificateNotificationManager.java
Outdated
Show resolved
Hide resolved
store/src/main/java/com/zimbra/cs/service/admin/CertificateNotificationManager.java
Outdated
Show resolved
Hide resolved
store/src/main/java/com/zimbra/cs/service/admin/CertificateNotificationManager.java
Outdated
Show resolved
Hide resolved
store/src/test/java/com/zimbra/cs/service/admin/IssueCertTest.java
Outdated
Show resolved
Hide resolved
store/src/test/java/com/zimbra/cs/service/admin/IssueCertTest.java
Outdated
Show resolved
Hide resolved
store/src/main/java/com/zimbra/cs/service/admin/CertificateNotificationManager.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor details to fix, the implementation seems ok!
What's changed:
Current flow:
The System is processing your certificate generation request. It will send the result to the Domain notification recipients.
we have 4 scenarios : №1 system failures (not related to certbot), №2 certbot failures, №3 certbot success (certificate recieved or certificate not yet due for renewal), №4 other (smth is wrong and wasn’t expected)
Related PRs:
zextras/carbonio-core-utils#38
https://github.com/Zextras/carbonio-build/pull/50
https://bitbucket.org/zextras/carbonio-core/pull-requests/42