Skip to content
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

Email will not be sent without any failure #8665

Merged
merged 1 commit into from
Sep 6, 2023

Conversation

VaibhavMalik4187
Copy link
Contributor

@VaibhavMalik4187 VaibhavMalik4187 commented Sep 6, 2023

Removed extra string: EMAIL_BODY= from the environment variable EMAIL_BODY.

Fixes: #8664

Signed commits

  • Yes, I signed my commits.

@github-actions github-actions bot added the area/ci Continuous Integration label Sep 6, 2023
@Ghat0tkach
Copy link
Member

@VaibhavMalik4187 Let's discuss this on Meshery Dev call. Please add this as an agenda item in the meeting minutes if you would. :)

Copy link
Contributor

@suhail34 suhail34 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


I think the problem is here the condition should be something like this
if: ${ always() && (needs.update-grapql-docs.result == 'failure' || needs.update-swagger-docs.result=='failure'and so on for other jobs) }

@VaibhavMalik4187
Copy link
Contributor Author

I think the problem is here the condition should be something like this
if: ${ always() && (needs.update-grapql-docs.result == 'failure' || needs.update-swagger-docs.result=='failure'and so on for other jobs) }

This is yet another way to check if we should start the job or not. However, this looks very tedious, especially for workflows having multiple jobs.
Do you mind if I ask what is wrong with:

      - name: Send Email Notification
        if: ${{ env.EMAIL_BODY != '' }}

@suhail34
Copy link
Contributor

suhail34 commented Sep 6, 2023

Okay i got you now we can do that but if there are no jobs failed then why do we need to run email-on-failure we can stop it the start itself

@VaibhavMalik4187
Copy link
Contributor Author

Okay i got you now we can do that but if there are no jobs failed then why do we need to run email-on-failure we can stop it the start itself

Your approach suggests checking for failed jobs by hard coding the values and then comparing the result of each job with "failure". This violates the DRY principle. It is also prone to error. Suppose, later, at some point, 5 more jobs are added in this workflow. The comparisons would keep on growing. That's why I'm using the REST API to determine the failed jobs without hardcoding any values. And just as you suggested, I'm also skipping the Send email step if none of the jobs have failed.

@suhail34
Copy link
Contributor

suhail34 commented Sep 6, 2023

Yes you are right that was just a suggestion what i meant was to check if there was a failure in any of the previously runned job then only email-on-failure job should run we can also try
if: failure() so that it will run only on failure of previously runned job

@VaibhavMalik4187 VaibhavMalik4187 force-pushed the email-on-failure-bug-fix branch 4 times, most recently from 6aa91f3 to 33ad3b4 Compare September 6, 2023 11:38
Removed extra string: `EMAIL_BODY=` from the environment variable
EMAIL_BODY.

Fixes: meshery#8664

Signed-off-by: VaibhavMalik4187 <vaibhavmalik2018@gmail.com>
@VaibhavMalik4187
Copy link
Contributor Author

Yes you are right that was just a suggestion what i meant was to check if there was a failure in any of the previously runned job then only email-on-failure job should run we can also try if: failure() so that it will run only on failure of previously runned job

Great suggestion, I've changed the code accordingly.

Copy link
Contributor

@suhail34 suhail34 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👌

@suhail34 suhail34 merged commit 18ecc96 into meshery:master Sep 6, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ci Continuous Integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

email-on-failure job sends email even when there is no failure
3 participants