AWS parser update #244
AWS parser update #244
Conversation
gskjelstad
requested review from
chadell,
glennmatthews,
pke11y,
scetron and
jvanderaa
as code owners
| search = re.search(r"\[AWS Account ?I?D?: ([0-9]+)\]", subject) | ||
| if search: | ||
| data["account"] = search.group(1) | ||
| data = {"account": ""} |
There was a problem hiding this comment.
If we can't find the "account", we should skip adding the key with an empty value. It's better to fail clearly than have a result like the one in tests/unit/data/aws/aws3_subject_parser_result.json
There was a problem hiding this comment.
I guess that's the rub in this case. Some of the emails have the account number in the subject line, while others don't, (it's in the body). If I just skip adding an empty key, there is no other data thats parsed on subject line, and it returns an empty data structure [{}] this breaks the upstream parser tho, as it considers it to have failed parsing vs. an expected empty response. Maybe I'm not understanding it clearly tho' - what would be a good practice to do here when you are EXPECTING there to be no data under certain conditions?
There was a problem hiding this comment.
the Providers (e.g., aws), contain different Processors, which are a combination of one or more Parsers.
The way the circuit maintenance parser works is to try to extract part of the Maintenance data from each Parser and then finally combine the data to create a valid Maintenance.
This means that is some Processor has a Parser that is not capturing the proper data it's ok, another one could do in the same process.
There was a problem hiding this comment.
Alright cool, I think after incorporating the feedback you gave me, I think we probably are ready to give this a look over again. Let me know what you think! 👍
| @@ -186,6 +186,7 @@ class AWS(GenericProvider): | |||
| """AWS provider custom class.""" | |||
|
|
|||
| _processors: List[GenericProcessor] = [ | |||
| CombinedProcessor(data_parsers=[EmailDateParser, TextParserAWS1]), | |||
There was a problem hiding this comment.
have you tried to invert them? so we test first the most complete parsing, including the Subject, and only then it fails, we try the second approach.
There was a problem hiding this comment.
yep! I think we had even hinted in that direction, to ensure that the more complete parser would take precedent in it's processing. Thanks for the reminder! I've swapped those!
| # there may still be some strings with data to capture. | ||
| # otherwise, continue on. | ||
| if not line_matched: | ||
| if re.match(r"[a-z]{5}-[a-z0-9]{8}", line): |
There was a problem hiding this comment.
just curious why haven't you added this Regex to the others?
| # Common Subject strings for matching: | ||
| subject_map = [{"account": r"\[AWS Account ?I?D?: ([0-9]+)\]"}] | ||
|
|
||
| subject_list = [] |
There was a problem hiding this comment.
this is the exactly same code you have in the other function, extract this to a staticmethod to reuse
First PR for feedback and assistance with building/designing tests! 🙏