Skip to content
This repository has been archived by the owner on Jun 5, 2023. It is now read-only.

Make email sending optional. #147

Merged
merged 11 commits into from
Mar 24, 2017
Merged

Make email sending optional. #147

merged 11 commits into from
Mar 24, 2017

Conversation

blueandgold
Copy link
Contributor

@blueandgold blueandgold commented Mar 24, 2017

Based on user feedback, we want to make the email to be optional, especially when running initially for evaluation purposes.

The only other place where the email comes up is in the DM template. Will we assume that email notifications will be required there? If no, i can test it to make sure the optional case is handled correctly.
https://github.com/GoogleCloudPlatform/forseti-security/blob/master/deployment-templates/py/forseti-instance.py

Fixes #146

@carise
Copy link
Contributor

carise commented Mar 24, 2017

re: your question about DM -- is it possible to test whether the email address properties are present in the context.properties and conditionally add them as commandline flags in the startup script?

@blueandgold
Copy link
Contributor Author

I was concerned about that, too. Let me verify how it behaves.

@blueandgold
Copy link
Contributor Author

blueandgold commented Mar 24, 2017

PTAL, there's a bit more change to make sure we are handling the case when someone does not want to send email in the VM environment. In this case, I am assuming the email properties are not specified. In which case, a string 'None' will need to be handled since None will be formatted to 'None'.

Let me know if there's a better way this can be handled.

@carise
Copy link
Contributor

carise commented Mar 24, 2017

I'm ok with the current changes, but maybe what you could also do for the DM is create a separate string for the email flags (instead of --sendgrid-api-key {} --email-sender {} etc. just have a {} and string replace the value with either a blank or the full list of flags/values), perhaps?

@blueandgold
Copy link
Contributor Author

Okay, I shuffled the strings around a bit in the DM template. How does it look now?

Copy link
Contributor

@carise carise left a comment

Choose a reason for hiding this comment

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

💯

@blueandgold
Copy link
Contributor Author

Adding cla: yes label, as the PR author is a googler at the time of this PR was created.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sending email should be optional
2 participants