-
Notifications
You must be signed in to change notification settings - Fork 54
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
141 update failure notification #144
141 update failure notification #144
Conversation
Should I add constants with new key strings? |
@WallysFerreira yes - lets move them to constants |
src/emailalert/email_function.py
Outdated
result[constants.ERROR] = event['taskname'] + 'Error' | ||
result[constants.CAUSE] = event['status'] | ||
result['failed step'] = event['taskname'] + 'Error' | ||
result['cause of failure'] = event['status'] |
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.
lets use constants for these
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.
have added some comments. please address
@WallysFerreira as part of the PR description, could you also add a screenshot of the new email and slack notification post your changes? |
Yes I can do that |
Maybe I jumped some step on the tutorial, but when it tries to deploy to AWS it returns Error: Template file not found at /home/wf/Trapheus/deploy.yaml |
@WallysFerreira do reverify your setup and a give it a try again. If it doesnt work, let me know - i can try running your changes locally to verify the output. |
I've tried running on a codespace and it's still returning an error, even though there is a template.yaml and deploy.yaml on the directory. FileNotFoundError: [Errno 2] No such file or directory: 'sam package --template-file template.yaml --output-template-file deploy.yaml --s3-bucket teste-741284363623 --region us-east-2' |
Ok.. give me a day or two. Will try this locally and get back to you. |
@WallysFerreira i ran your changes locally. but i get the following error:
can we check why also, i faced the same issue as you did in trying to run trapheus locally - some recent changes have broken the install.py file. |
Yeah sure I'll look into it and use the new install.py. Thanks |
@WallysFerreira i have merged the fix for install.py. can you pull the latest on your branch and try - no need to use a separate version of the file |
I think the error was caused by the capital 'i' in identifier, but can't actually check because I'm getting another error when running install.py Error: Unable to upload artifact src/common/common.zip referenced by ContentUri parameter of CommonLambdaLayer resource. |
@WallysFerreira looks like your temporary aws credentials may have expired. could you refresh and try again? |
I tried with new aws credentials and still got the same error :( |
@namitad / @WallysFerreira there is a couple of issues in the way this is implemented. The way we are getting the identifier is a bit off. The event that gets generated is this
As you can see there is no direct way of getting the Identifier as a key. We have a utilty function defined that needs to be utilised https://github.com/intuit/Trapheus/blob/master/src/common/python/utility.py#L33 to get the identifier from the event. Secondly, the way With the above changes, you guys should be able to get that out. Let me know if you need help |
@WallysFerreira it would still run into a similar issue (as mentioned by @stationeros above) with the "identifier" change. we need to explicitly pass the identifier name to the state where alerts are being sent. Can we look at using a pass state https://docs.aws.amazon.com/step-functions/latest/dg/amazon-states-language-pass-state.html to add this to the state machine template.yaml? This can also be used in the future if we need to add additional values - such as execution id. |
@stationeros yes - taskname is being cascaded because in the context object - it would either be the email alert/slack notification lambda name. even if set from each step, it would expose the lambda function name to the user instead of the flow. we can revisit if this change needs to be done and maybe address that as a separate issue. |
@stationeros that's super helpful, thanks. I'll make those changes asap. @namitad I'll try to look into using a pass state afterwards, but it would still work without it if I make the changes @stationeros said, right? Just so I can do one thing at a time |
@WallysFerreira Thats right? You can possibly fix the instance identifier and the task name chaining can be done later. I can break this into subtasks for this to be managed better |
@WallysFerreira Can you make the code change for instance identifier , you just need to call this inside the lambda , we can then merge this commit |
@WallysFerreira Closing this PR due to inactivity, feel free to re-open and submit if you are still working on this. |
Description
Change result message of email and slack notification in errors