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

Don't pass invalid auth method "None" to net-smtp #12384

Merged
merged 5 commits into from Apr 18, 2024

Conversation

mkllnk
Copy link
Member

@mkllnk mkllnk commented Apr 16, 2024

What? Why?

The indirect dependency net-smtp introduced a new check on authentication methods. Suddenly it raised an error on our non-standard auth method "None" which indicates not to authenticate.

What should we test?

  • Send an email on au-staging.
  • Send an email on another staging server.
  • You should receive both emails and not see anything in Bugsnag.

Release notes

Changelog Category (reviewers may add a label for the release notes):

  • User facing changes
  • API changes (V0, V1, DFC or Webhook)
  • Technical changes only
  • Feature toggled

The title of the pull request will be included in the release notes.

Dependencies

Documentation updates

@mkllnk mkllnk self-assigned this Apr 16, 2024
It's our magic word to not pass username and password on.
Instead of using the auth method name, let's just not supply username
and password when we don't want to authenticate. The two affected
servers AU and CA don't have credentials set anyway. This is compatible.

The specs needed changing though.
We were always adding this option anyway, so why not declare it to start
with?
We can simply merge the option hashes now because they are not
conditional anymore. Well, the magic `presence` method does the
conditional logic for us now.
Best viewed with whitespace ignored.
@mkllnk mkllnk added the user facing changes Thes pull requests affect the user experience label Apr 16, 2024
@mkllnk mkllnk marked this pull request as ready for review April 16, 2024 06:55
Copy link
Collaborator

@rioug rioug left a comment

Choose a reason for hiding this comment

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

Nice one, It's always nice to see the code simplified.

Copy link
Member

@dacook dacook left a comment

Choose a reason for hiding this comment

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

Great, thanks for fixing this.
It's nice how improving code creates opportunity to simplify it further, thanks for going ahead and cleaning up so much! 😀

@dacook dacook self-assigned this Apr 18, 2024
@dacook dacook added pr-staged-uk staging.openfoodnetwork.org.uk pr-staged-au staging.openfoodnetwork.org.au labels Apr 18, 2024
@dacook
Copy link
Member

dacook commented Apr 18, 2024

I'll go ahead and test this so it can be included in the release.

@dacook
Copy link
Member

dacook commented Apr 18, 2024

Had to fix separate unrelated issues on each staging server.

au_staging

https://staging.openfoodnetwork.org.au/admin/mail_methods/edit > Send test email
✅ Successfully received. No Bugsnag errors. Email source:

...
X-Received: by 2002:a05:6602:2549:b0:7d3:55a4:ab3f with SMTP id cg9-20020a056602254900b007d355a4ab3fmr1593149iob.15.1713404423691;
        Wed, 17 Apr 2024 18:40:23 -0700 (PDT)
Return-Path: <orders+staging@openfoodnetwork.org.au>
Received: from smtp.gmail.com ([65.99.217.140])
        by smtp-relay.gmail.com with ESMTPS id x14-20020a026f0e000000b00481b24766c6sm49743jab.30.2024.04.17.18.40.21
        (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128);
        Wed, 17 Apr 2024 18:40:23 -0700 (PDT)
...

uk_staging

Broken, try another

fr_staging

Seems a bit unhealthy

  1. client_loop: send disconnect: Broken pipe after 6m 36s.
  2. "bundle exec rake assets:precompile RAILS_ENV=staging"], ... "msg": "non-zero return code" after 5m 28s.
  3. client_loop: send disconnect: Broken pipe after 6m 18s.

@dacook dacook added pr-staged-fr staging.coopcircuits.fr and removed pr-staged-uk staging.openfoodnetwork.org.uk labels Apr 18, 2024
@dacook
Copy link
Member

dacook commented Apr 18, 2024

fr_staging

  1. Finally, success after 5m 29s

✅ Successfully received. No Bugsnag errors. Email source:

Received: by recvd-5d958566ff-2tqqg with SMTP id recvd-5d958566ff-2tqqg-1-6620C1F1-10 2024-04-18 06:47:13.857602197 +0000 UTC m=+463654.459376259
Received: from coopcircuits.fr (unknown) by geopod-ismtpd-0 (SG) with ESMTP id BmckWptOQxadziil2z65BA for <david@openfoodnetwork.org.au>; Thu, 18 Apr 2024 06:47:13.742 +0000 (UTC)

@dacook dacook removed pr-staged-au staging.openfoodnetwork.org.au pr-staged-fr staging.coopcircuits.fr labels Apr 18, 2024
@dacook dacook merged commit 5b04357 into openfoodfoundation:master Apr 18, 2024
57 of 58 checks passed
@mkllnk mkllnk deleted the mail-config branch May 8, 2024 00:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
user facing changes Thes pull requests affect the user experience
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

ArgumentError in ActionMailer::MailDeliveryJob@default
3 participants