[email-sender] introduce new integration #418
Conversation
| raise KeyError( | ||
| f"[state] key {key} already exists in {self.state_path}") | ||
| self.client.put_object( | ||
| Bucket=self.bucket, Key=f"{self.state_path}/{key}") |
There was a problem hiding this comment.
I like this class. It's clean and direct to the point. I'm thinking... how about using __setitem__ and __contains__ (__getitem__ might also be useful) to implement these functionalities here? Anyway, it looks good as is.
There was a problem hiding this comment.
So, this is what I have in mind:
class Status:
def __init__(self):
"""
Status
"""
def __getitem__(self, item):
"""
Returns the value of an object:
client.get_object()['Body'].read()
If the object body is a JSON serialized content (which I think it's
a good idea), this returns:
json.loads(client.get_object()['Body'].read())
Raises KeyError when the object does not exist.
Usage:
foo = status['foo']
"""
def __setitem__(self, key, value):
"""
Stores an object. I'd use:
put_object(..., Body=json.dumps(value))
Usage:
status['foo'] = None
"""
def __delitem__(self, key):
"""
Deletes an object.
Raises KeyError when the object does not exist.
Usage:
del(status['foo'])
"""
def __contains__(self, item):
"""
Checks if an object exists (client.head_object()).
Usage:
if 'foo' in status:
# do something
Returns True/False.
"""
def __len__(self):
"""
Returns the number of objects. Probably the best approach
is to use boto3.resource.Bucket.objects.all()
Usage:
len(status)
"""
def __iter__(self):
"""
Creates a generator that yields all the keys. Probably the
best approach is to use boto3.resource.Bucket.objects.all()
Usage:
for key in status:
print(key)
"""
def keys(self):
"""
Returns list(self), which takes advantage of __iter__.
Usage:
for key in status.keys():
print(key)
"""
def values(self):
"""
Returns [self[key] for key in self.keys()], which takes advantage
of __getitem__ and self.keys().
Usage:
for value in status.values():
print(value)
"""
def items(self):
"""
For key in self.keys(), this yields (key, self[key]).
Usage:
for key, value in status.items():
print(key, value)
"""
def add(self, key):
"""
Note: not sure this is needed, but in your use case, you are adding
keys without values, so this method might be useful.
Adds a key without specifying a value (mimicking a set). I'd
use this to just wrap self[key] = None
Usage:
status.add('foo')
if 'foo' in status:
print('Yay!')
"""There was a problem hiding this comment.
I totally agree with this suggestion and the proposed solution.
that said, we currently only maintain a state of happened/did not happen. this will be the first feature we do when we have an integration that needs state with content. i've added an issue for it: #419.
I'll keep this class as is, as your first comment says that as well :)
There was a problem hiding this comment.
Sure, thank you! I didn't mean to implement all the interfaces... just those needed at this point, which are the add() and the __contains__. Thak you for creating the issue.
|
Very nice. A couple of comments, but overwall I like it a lot. |
This PR introduces a new integration that allows us to send emails through app-interface.
Matching app-interface PR: https://gitlab.cee.redhat.com/service/app-interface/merge_requests/2531
The biggest challenge was: how do we only send each email only once.
The proposed solution: we use S3 as a simple state for app-interface.
This approach will allow us to implement new integrations that do not have a source of truth that we can fetch current state from. For example, this will allow running SQL queries only once (RDS self service).