-
Notifications
You must be signed in to change notification settings - Fork 83
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 notifications for create,update,delete deployment #182
Email notifications for create,update,delete deployment #182
Conversation
Thank you for raising the request! RAD Lab admins have been notified. |
Apache 2.0 License check successful! |
-Feature: Enable Email notifications to Trusted and Owner Users / Groups for RAD Lab module updates
|
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.
It's unclear to me what to provide as "Mail Box Email" and "Mail Box Password". I think we need to document this much more (in a subsection under Troubleshooting) and we need to link to that on the admin variable page; otherwise, it's unclear what to put here.
userManaged: { | ||
replicas: [ | ||
{ | ||
location: "us-central1", |
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.
Do we need this region? If so, it can't be hard coded. It needs to be set as an environment variable and documented here: https://googlecloudplatform.github.io/rad-lab/docs/rad-lab-ui/ui_installation/env-variables/
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.
Yes its required hence storing it as env and updated the document.
Co-authored-by: Alan Colver <[email protected]>
Co-authored-by: Alan Colver <[email protected]>
Co-authored-by: Alan Colver <[email protected]>
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.
Will you please write documentation as indicated in my previous review?
const saveMailBoxCred = async (payload: ISecretManagerReq) => { | ||
try { | ||
await axios.post("/api/secret", payload) | ||
} catch (error: any) { |
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.
Can we put a proper type here? I believe it's some sort of AxiosErrorResponse
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.
catch
clause accept only any
or unknown
type hence used any
here
await configureEmailAndSend( | ||
"RAD Lab Module has been created for you!", | ||
response, | ||
) |
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.
These probably should've been done as Pub/Sub topic publishes, and the email sending as a Subscriber. If we have to do another async feature like this, we will rewrite both to Pub/Sub.
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.
No action needed at this time.
}, | ||
}) | ||
|
||
transporter.verify().then(console.log).catch(console.error) |
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.
I think you need to await
this. And do we need to console.log
on success?
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.
Updated to await
and removed then
block
Co-authored-by: Alan Colver <[email protected]>
Co-authored-by: Alan Colver <[email protected]>
Co-authored-by: Alan Colver <[email protected]>
@@ -4,3 +4,20 @@ sidebar_position: 4 | |||
# Troubleshooting | |||
|
|||
Please feel free to reach out and to create a [GitHub issue](https://github.com/GoogleCloudPlatform/rad-lab/issues) in case of any issues or concerns. | |||
|
|||
## Email Notifications |
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.
I think the best place for this section is under Global Admin Variables section compared to Troubleshooting section.
And also we should remove the mention of Preferred Google Cloud Region and Zone
from Global Admin Variables.
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.
Moved Email Notifications
section to Global Admin Variables
@@ -35,6 +35,7 @@ Firebase environment variables can be found in project settings section of your | |||
- `NEXT_PUBLIC_NOTIFICATION_TOPIC` - Topic created for notifications | |||
- `NEXT_PUBLIC_NOTIFICATION_SUB` - Subscription created for notifications | |||
- `NEXT_PUBLIC_GIT_API_URL` - Url of the public repo to pull the deployment modules | |||
- `SECRET_MANAGER_LOCATION` - Google Cloud region of the Secret Manager instance |
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.
2 Questions:
-
How are we setting this Environment variable ? Are we needing admins to manually update the .env file with the region before UI deployment or using the existing bash script which creates the .env file.
-
What is the default value of this variable ? Is it the same as app_engine_location variable ?
SOLUTION: If its manually set and we want to automate it like other variables we can expose the above app_engine_location
variable as a TF output and we can use the existing bash script to create the .env with the same.
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.
app_engine_location
and Secret Manager Location
are different things so created new env in outputs
and updated in bash script
file.
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.
I have added my feedback / review. There may be some changes needed to automatically set SECRET_MANAGER_LOCATION
env variable, Please let me know if you need any clarifications.
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.
Everything looks good! Thanks for working on this guys :)
Release for #141 |
No description provided.