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

Update build #3017

Merged
merged 1 commit into from
Jul 25, 2022
Merged

Update build #3017

merged 1 commit into from
Jul 25, 2022

Conversation

SuperQ
Copy link
Member

@SuperQ SuperQ commented Jul 23, 2022

  • Update Go to 1.18
  • Update circleci machine image.
  • Switch maildev to new upstream image location.
  • Update Go modules to 1.17 format.
  • Make dependabot monthly to match prometheus/prometheus.

Signed-off-by: SuperQ superq@gmail.com

@SuperQ SuperQ force-pushed the superq/build branch 2 times, most recently from 077951c to 213c305 Compare July 23, 2022 15:28
@SuperQ
Copy link
Member Author

SuperQ commented Jul 23, 2022

It looks like MailDev completely dropped STARTLS. (maildev/maildev#274)

* Update Go to 1.18
* Update circleci machine image.
* Switch maildev to new upstream image location.
* Update Go modules to 1.17 format.
* Make dependabot monthly to match prometheus/prometheus.

Signed-off-by: SuperQ <superq@gmail.com>
Copy link
Member

@gotjosh gotjosh left a comment

Choose a reason for hiding this comment

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

LGTM, but please see my comment re the go version.

go.mod Show resolved Hide resolved
@SuperQ SuperQ merged commit 155a47f into main Jul 25, 2022
@SuperQ SuperQ deleted the superq/build branch July 25, 2022 13:56
@joeblubaugh
Copy link
Contributor

joeblubaugh commented Sep 9, 2022

This PR also included a functional change to the email notifier, using x/text to do casing, and x/text panics on some inputs. Here's a stack trace we've seen while running the mimir alertmanager:

/vendor/gopkg.in/yaml.v2/decode.go:270 +0xa7
gopkg.in/yaml%2ev2.(*decoder).callUnmarshaler(0xc20be3f980, 0xc20c116e70, {0x7f170dda42b8, 0xc0430126c0})
/vendor/github.com/prometheus/alertmanager/config/notifiers.go:196 +0x1b9
github.com/prometheus/alertmanager/config.(*EmailConfig).UnmarshalYAML(0xc0430126c0, 0x2340b20?)
	/vendor/golang.org/x/text/cases/cases.go:51
golang.org/x/text/cases.Caser.String(...)
	/vendor/golang.org/x/text/transform/transform.go:650 +0xbe5
golang.org/x/text/transform.String({0x7f170e30ea18, 0xc0002d2a00}, {0xc20fd3aeb8, 0x2})
...
runtime error: slice bounds out of range [11:7]

That wasn't mentioned in the commit, making this a very difficult to track regression.

I'm going to file an issue asking to revert the email notifier changes. I think they should be done in a separate PR, and I don't understand the problem that the casing call is solving.

I also think we can get away without doing Title casing - Email Header names are case insensitive: https://www.rfc-editor.org/rfc/rfc5322#section-1.2.2 (see also: https://stackoverflow.com/questions/6143549/are-email-headers-case-sensitive), so lower-casing every header with strings.ToLower() should be sufficient to check for collisions. so the code could strings.ToLower() everything, which is a simpler transformation.

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.

None yet

3 participants