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

Notification Support for Workflow Approval Nodes #4657

Merged

Conversation

beeankha
Copy link
Contributor

@beeankha beeankha commented Sep 4, 2019

Summary

This feature enhancement will enable notifications to be sent out whenever an approval node needs to be approved, as well as whenever they get approved/denied/are timed out.

Follow-up to PR #4264.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME
  • API
  • UI
AWX VERSION
awx: 6.1.0

@softwarefactory-project-zuul
Copy link
Contributor

Build failed.

@softwarefactory-project-zuul
Copy link
Contributor

Build failed.

@softwarefactory-project-zuul
Copy link
Contributor

Build failed.

@softwarefactory-project-zuul
Copy link
Contributor

Build failed.

@beeankha beeankha force-pushed the wf_approval_notification branch from 7e08bfc to 93831b4 Compare September 7, 2019 02:07
@softwarefactory-project-zuul
Copy link
Contributor

Build failed.

@beeankha beeankha force-pushed the wf_approval_notification branch from 93831b4 to 570aa1e Compare September 10, 2019 15:07
@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@softwarefactory-project-zuul
Copy link
Contributor

Build failed.

@beeankha
Copy link
Contributor Author

recheck

migrations.AlterField(
model_name='workflowjobnode',
name='do_not_run',
field=models.BooleanField(default=False, help_text='Indicates that a job will not be created when True. Workflow runtime semantics will mark this True if the node is in a path that will decidedly not be ran. A value of False means the node may not run.'),
Copy link
Contributor

Choose a reason for hiding this comment

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

If anybody else notices this, looks like it's just a help_text typo.

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

.filter(unifiedjobtemplate_notification_templates_for_approvals__in=[self]))
# Get Organization NotificationTemplates
if self.organization is not None:
error_notification_templates = set(error_notification_templates + list(base_notification_templates.filter(
Copy link
Contributor

@ryanpetrello ryanpetrello Sep 10, 2019

Choose a reason for hiding this comment

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

Prior to this commit, it looks like the failure, success, and started notification templates for workflows don't inherit from the Organization. Maybe that could be considered a bug (cc @AlanCoding @wenottingham)? I think for the purposes of this PR, though, we probably don't want to introduce that behavioral change.

We do, however, want to keep the line you added for inheriting approval notifications from the org because that is something we've explicitly said we want to support w/ this new feature:

approval_notification_templates = set(
    approval_notification_templates + list(base_notification_templates.filter(
        # etc...

Copy link
Contributor

Choose a reason for hiding this comment

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

Prior to this commit, it looks like the failure, success, and started notification templates for workflows don't inherit from the Organization. Maybe that could be considered a bug

fwiw, sounds like a bug to me

Copy link
Contributor Author

@beeankha beeankha Sep 11, 2019

Choose a reason for hiding this comment

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

I've confirmed that without the

error_notification_templates = set(error_notification_templates + list(base_notification_templates.filter(
    organization_notification_templates_for_errors=self.organization)))
started_notification_templates = set(started_notification_templates + list(base_notification_templates.filter(
    organization_notification_templates_for_started=self.organization)))
success_notification_templates = set(success_notification_templates + list(base_notification_templates.filter(
    organization_notification_templates_for_success=self.organization)))

code, the "start" "success" and "error" notifications do NOT send on an org level. Will fix this bug in a separate PR, per discussion above.

Copy link
Contributor

Choose a reason for hiding this comment

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

@matburt @wenottingham can you confirm that this is a bug, or is there a reason for this behavior?

Copy link
Contributor

@wenottingham wenottingham Sep 11, 2019

Choose a reason for hiding this comment

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

Is this for workflows with an explicit org? It would appear to be a bug for that case, but I also believe you can still have orgless workflows.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, we think we've discovered that WFJTs w/ an explicit org will not inherit Notification Templates from that org (at all).

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's treat is as a bug and track it as separate work @beeankha @jladdjr

def send_approval_notification(self, status):
from awx.main.tasks import send_notifications # avoid circular import
if self.workflow_job_template is None:
return
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 good idea

subject.append((' was denied. {}').format(workflow_url))
subject = " ".join(subject)
body = self.notification_data()
body['body'] = subject
Copy link
Contributor

@ryanpetrello ryanpetrello Sep 10, 2019

Choose a reason for hiding this comment

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

This looks great to me 👍 @jladdjr you have any other thoughts here?

Copy link
Contributor

Choose a reason for hiding this comment

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

@ryanpetrello - @beeankha and I took a look at this last week - this looks like it's doing the right thing 👍

url = reverse('api:workflow_job_template_notification_templates_approvals_list', kwargs={'pk': workflow_job_template.pk})
response = get(url, admin)
assert response.status_code == 200
assert len(response.data['results']) == 0
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

assert response.status_code == 204
response = get(url, admin)
assert response.status_code == 200
assert len(response.data['results']) == 1
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

url = reverse('api:organization_notification_templates_approvals_list', kwargs={'pk': organization.pk})
response = get(url, admin)
assert response.status_code == 200
assert len(response.data['results']) == 0
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@beeankha beeankha force-pushed the wf_approval_notification branch from f942d4c to 3182197 Compare September 27, 2019 19:48
@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded (gate pipeline).

@softwarefactory-project-zuul softwarefactory-project-zuul bot merged commit 505dcf9 into ansible:devel Sep 30, 2019
@beeankha beeankha changed the title [WIP] Notification Support for Workflow Approval Nodes Notification Support for Workflow Approval Nodes Sep 30, 2019
@jbradberry jbradberry mentioned this pull request Mar 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants