fix(smtp): use `usestarttls` (match the docs) #252

JosephKav · 2022-05-26T23:05:53Z

The smtp docs mention UseStartTLS, but it doesn't accept usestarttls, it wants starttls. As UseStartTLS is the Go var, it was probably designed to be usestarttls as the key, so here's a PR for that.

Thanks to @samcro1967 for spotting this in the issue for my PR that added Shoutrrr
release-argus/Argus#33 (comment)

Wasn't sure about how/whether you'd want to keep support for starttls for a while as it would show in the docs if I put it under keys, right?

codecov · 2022-05-26T23:49:31Z

Codecov Report

Merging #252 (66cb389) into main (468d552) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #252   +/-   ##
=======================================
  Coverage   77.73%   77.73%           
=======================================
  Files          96       96           
  Lines        2866     2866           
=======================================
  Hits         2228     2228           
  Misses        468      468           
  Partials      170      170

Impacted Files	Coverage Δ
pkg/services/smtp/smtp_config.go	`87.09% <ø> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 468d552...66cb389. Read the comment docs.

JosephKav · 2022-05-26T23:50:15Z

pkg/services/smtp/smtp_test.go

@@ -42,9 +43,10 @@ var _ = Describe("the SMTP service", func() {
 	})
 	When("parsing the configuration URL", func() {
 		It("should be identical after de-/serialization", func() {
-			testURL := "smtp://user:password@example.com:2225/?auth=None&encryption=ExplicitTLS&fromaddress=sender%40example.com&fromname=Sender&starttls=No&subject=Subject&toaddresses=rec1%40example.com%2Crec2%40example.com&usehtml=Yes"
+			testURL := "smtp://user:password@example.com:2225/?auth=None&encryption=ExplicitTLS&fromaddress=sender%40example.com&fromname=Sender&subject=Subject&toaddresses=rec1%40example.com%2Crec2%40example.com&usehtml=Yes&usestarttls=No"


Just noticed why I had to move this to the end. This is Go doing its usual alphabetising of a map right?

No, actually Go randomizes the map order. But to make the tests repeatable, we sort the keys in alphabetical order.

ah, must be the yaml encoder that alphabetises my maps then. Unless I have been extremely 'lucky' to have it alphabetised correctly every time I checked after a save

Indeed it does, it has it's own map impl: https://pkg.go.dev/gopkg.in/yaml.v2#MapSlice

I wish v3 has a MapSlice so that I could choose the order when I save it. Had to make it save the file, then go through and check whether the blocks have been written in the correct order (they won't have been...), then bubble sort those blocks back to how it should be and save again! I did spend a while on ordering the maps, but then found out they were alphabetised and am calling that good enough

email was using `toaddress` instead of `toaddresses` added internal conversion from `usestarttls` to `starttls` for `notify` containrrr/shoutrrr#252

piksel

Thanks for finding this!

It's an oversight in the docs generation. When the key doesn't match the field name it should make a note of that, instead of just assuming they will be same.

In any case, this looks good, apart from the breaking change, but that's easily fixed.

piksel · 2022-05-27T09:58:48Z

pkg/services/smtp/smtp_test.go

@@ -42,9 +43,10 @@ var _ = Describe("the SMTP service", func() {
 	})
 	When("parsing the configuration URL", func() {
 		It("should be identical after de-/serialization", func() {
-			testURL := "smtp://user:password@example.com:2225/?auth=None&encryption=ExplicitTLS&fromaddress=sender%40example.com&fromname=Sender&starttls=No&subject=Subject&toaddresses=rec1%40example.com%2Crec2%40example.com&usehtml=Yes"
+			testURL := "smtp://user:password@example.com:2225/?auth=None&encryption=ExplicitTLS&fromaddress=sender%40example.com&fromname=Sender&subject=Subject&toaddresses=rec1%40example.com%2Crec2%40example.com&usehtml=Yes&usestarttls=No"


No, actually Go randomizes the map order. But to make the tests repeatable, we sort the keys in alphabetical order.

pkg/services/smtp/smtp_config.go

piksel · 2022-05-27T10:32:44Z

@all-contributors add @JosephKav for bug

allcontributors · 2022-05-27T10:32:52Z

@piksel

I've put up a pull request to add @JosephKav! 🎉

- `url_fields.username`, not `params.username` - fix some errors to show it's in params, not url_fields - remove internal conversion to `starttls` for email as shoutrrr fixed it containrrr/shoutrrr#252

…77) * build(deps): bump github.com/containrrr/shoutrrr from 0.5.3 to 0.6.0 Bumps [github.com/containrrr/shoutrrr](https://github.com/containrrr/shoutrrr) from 0.5.3 to 0.6.0. - [Release notes](https://github.com/containrrr/shoutrrr/releases) - [Changelog](https://github.com/containrrr/shoutrrr/blob/main/goreleaser.yml) - [Commits](containrrr/shoutrrr@v0.5.3...v0.6.0) --- updated-dependencies: - dependency-name: github.com/containrrr/shoutrrr dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> * fix(notify): mattermost username harddefault + error printing - `url_fields.username`, not `params.username` - fix some errors to show it's in params, not url_fields - remove internal conversion to `starttls` for email as shoutrrr fixed it containrrr/shoutrrr#252 - `-test.notify` uses defaults+harddefaults Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Joseph Kavanagh <Joseph@kav.io>

fix(smtp): use usestarttls (match the docs)

e3afab5

JosephKav commented May 26, 2022

View reviewed changes

piksel requested changes May 27, 2022

View reviewed changes

fix: starttls as alias to prevent a breaking change

66cb389

JosephKav requested a review from piksel May 27, 2022 10:25

piksel approved these changes May 27, 2022

View reviewed changes

piksel merged commit 98cdc23 into containrrr:main May 27, 2022

allcontributors bot mentioned this pull request May 27, 2022

docs: add JosephKav as a contributor for bug #253

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(smtp): use `usestarttls` (match the docs) #252

fix(smtp): use `usestarttls` (match the docs) #252

JosephKav commented May 26, 2022 •

edited

codecov bot commented May 26, 2022 •

edited

JosephKav May 26, 2022 •

edited

piksel May 27, 2022

JosephKav May 27, 2022

piksel May 27, 2022

JosephKav May 27, 2022

piksel left a comment

piksel May 27, 2022

piksel commented May 27, 2022

allcontributors bot commented May 27, 2022

fix(smtp): use usestarttls (match the docs) #252

fix(smtp): use usestarttls (match the docs) #252

Conversation

JosephKav commented May 26, 2022 • edited

codecov bot commented May 26, 2022 • edited

Codecov Report

JosephKav May 26, 2022 • edited

Choose a reason for hiding this comment

piksel May 27, 2022

Choose a reason for hiding this comment

JosephKav May 27, 2022

Choose a reason for hiding this comment

piksel May 27, 2022

Choose a reason for hiding this comment

JosephKav May 27, 2022

Choose a reason for hiding this comment

piksel left a comment

Choose a reason for hiding this comment

piksel May 27, 2022

Choose a reason for hiding this comment

piksel commented May 27, 2022

allcontributors bot commented May 27, 2022

fix(smtp): use `usestarttls` (match the docs) #252

fix(smtp): use `usestarttls` (match the docs) #252

JosephKav commented May 26, 2022 •

edited

codecov bot commented May 26, 2022 •

edited

JosephKav May 26, 2022 •

edited