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

These changes to be able to run on GCLOUD #9043

Merged
merged 3 commits into from
Mar 30, 2021
Merged

These changes to be able to run on GCLOUD #9043

merged 3 commits into from
Mar 30, 2021

Conversation

icarito
Copy link
Member

@icarito icarito commented Jan 19, 2021

This were changes required to deploy on GCLOUD.

They are pointing to environment variables mostly.
Beware that the SMTP_* variables break in production, we've tried this before.
We should agree if we can try to deliver directly to Google SMTP, and possibly build the array procedurally (the issue is that empty string changes the actionmailer default and causes issues with current setup).

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

  • PR is descriptively titled 📑 and links the original issue above 🔗
  • tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with rake test
  • code is in uniquely-named feature branch and has no merge conflicts 📁
  • screenshots/GIFs are attached 📎 in case of UI updation
  • ask @publiclab/reviewers for help, in a comment below

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If tests do fail, click on the red X to learn why by reading the logs.

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software

Thanks!

@icarito icarito requested a review from a team as a code owner January 19, 2021 18:06
@gitpod-io
Copy link

gitpod-io bot commented Jan 19, 2021

@codecov
Copy link

codecov bot commented Jan 19, 2021

Codecov Report

❗ No coverage uploaded for pull request base (main@ff3417a). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 2fced08 differs from pull request most recent head 66c5940. Consider uploading reports for the commit 66c5940 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##             main    #9043   +/-   ##
=======================================
  Coverage        ?   79.68%           
=======================================
  Files           ?       98           
  Lines           ?     5950           
  Branches        ?        0           
=======================================
  Hits            ?     4741           
  Misses          ?     1209           
  Partials        ?        0           

@codeclimate
Copy link

codeclimate bot commented Jan 19, 2021

Code Climate has analyzed commit 76fdc18 and detected 0 issues on this pull request.

View more on Code Climate.

@jywarren
Copy link
Member

This looks good - do you think we should do this in stages, coordinated so we're watching this in production? Or are you OK merging this and monitoring the deployment asynchronously? Thank you, Sebastian!!!

@icarito
Copy link
Member Author

icarito commented Jan 20, 2021

This looks good - do you think we should do this in stages, coordinated so we're watching this in production? Or are you OK merging this and monitoring the deployment asynchronously? Thank you, Sebastian!!!

I expect the SMTP variables will cause some trouble if deployed like this.
The default values of SMTP_USER, SMTP_PASS, SMTP_AUTH, and SMTP_STLS break current configuration.

We have two options:

  • figure out correct values of the above for current configuration (plain local MTA)
  • point directly to deliver mail thru google (which our MTA ultimately does too)

The tradeoff is our /smtp_test monitoring route, which we'll loose as it's currently implemented because it doesn't support TLS/SSL.

@jywarren
Copy link
Member

Would you like me to try to refactor the smtp route for SSL? Is that at the Rails application level?

@icarito
Copy link
Member Author

icarito commented Mar 16, 2021

Hi Jeff,
I think what needs to be refactored is the way we build the config.action_mailer.smtp_settings hash.
The reason is that if we set user_name and password, authentication hash values to '' the result is not the same as when we omit these hash values.
So something like pseudocode:

if ENV[key]:
  config.action_mailer.smtp_settings[key] = ENV[key]

For each instance.
About the SMTP test, I believe it is not very effective to be honest so I would just drop it. Sentry report should be enough.

@jywarren jywarren merged commit d7a021d into main Mar 30, 2021
reginaalyssa pushed a commit to reginaalyssa/plots2 that referenced this pull request Oct 16, 2021
* These changes to be able to run on GCLOUD

* Give db variables to mailman & sidekiq

* Build hash list build strategy

Co-authored-by: root <root@plots2-central.c.public-lab.internal>
billymoroney1 pushed a commit to billymoroney1/plots2 that referenced this pull request Dec 28, 2021
* These changes to be able to run on GCLOUD

* Give db variables to mailman & sidekiq

* Build hash list build strategy

Co-authored-by: root <root@plots2-central.c.public-lab.internal>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants