Add Mail support to limes #667
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
majewsky
requested changes
Feb 18, 2025
Co-authored-by: Stefan Majewsky <[email protected]>
Co-authored-by: Stefan Majewsky <[email protected]>
Co-authored-by: Stefan Majewsky <[email protected]>
commitment amount in the second test case
mail template is now given as part of the cluster config
shorten commitmentQuery by using the AZResourceLocation data
Co-authored-by: Stefan Majewsky <[email protected]>
Co-authored-by: Stefan Majewsky <[email protected]>
Fix space in Duration attribute of test mail template
Add short-term notified query into process task Fix: Bug that already notified commitments would be notified on another run Add a unit test for the case above
fix: add subject to mail configuration. fix: change naming scheme from MailInfo to CommitmentGroupNotification
client type is a set value now change MailClient interface and implementation names and add documentation for those types
- make it private since it does not need to be public - load the entire ProjectCommitment object instead of just a few fields (loading everything is not very expensive, and not doing it feels odd) - fully prepare the mail instead of doing only half the work in the helper function
This is nominally more code, but the code in NewCluster() repeats itself less.
When suggesting slices.Collect() here earlier, I did not realize that the sort.Slice() does not do anything special. So this can be simplified further.
The inciting motivation here was to make the full ProjectCommitment object available to the template, like in e3daee2. This required splitting the discover phase into two separate queries, one for the commitment objects themselves and one for context joined from other tables. It then appeared natural to me to make the discover phase very lean and only load commitments there, and do all the remaining work in the processing phase.
majewsky
approved these changes
Feb 25, 2025
There was a problem hiding this comment.
I made another round of reviews as direct code commits; please counter-review these. If these look good, we're pretty close to done here. One last thing: Please add documentation.
docs/operators/config.mdshould explain the new required environment variable, and what kind of API we expect to have available under that endpoint.- Each public declaration (i.e. every upper-cased
type,func,var) should have at least one or two sentences of documentation, such thatgodocoutput or https://pkg.go.dev/github.com/sapcc/limes/internal/core etc. can be navigated in a useful way.
add godoc documentation for each public type, func, var
Also, upon further consideration, it is indeed weird that I wanted to have the mail endpoint coming in as an env var when the rest of the mail config is in the config file. So this commit groups everything together in the config file.
majewsky
force-pushed
the
mail_support
branch
from
b1b01d9 to
9a90e7d
Compare
majewsky
approved these changes
Feb 26, 2025
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR introduces mailing functionality to limes.
Confirmed and expired comitments should queue the commitment information for a project and send the email body to an API that is able to send those emails to their proper recipients.
Checklist: