Skip to content

Smoke Tests going through SES/S3#5549

Merged
mitchellhenke merged 7 commits intomainfrom
mitchellhenke/where-theres-smoke-tests
Oct 27, 2021
Merged

Smoke Tests going through SES/S3#5549
mitchellhenke merged 7 commits intomainfrom
mitchellhenke/where-theres-smoke-tests

Conversation

@mitchellhenke
Copy link
Copy Markdown
Contributor

No description provided.

@mitchellhenke mitchellhenke force-pushed the mitchellhenke/where-theres-smoke-tests branch 2 times, most recently from 83ae930 to 20f1bd4 Compare October 27, 2021 15:04
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

😭 😭 😭
Is it possible to generate one 30 seconds in the future and hope that our site allows some clock skew?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Or maybe should we just do backup codes and save the backup codes in memory? This seems like a very long time to wait

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's a bummer, but I don't super mind since smoke tests don't generally block PRs, and different environments can now run simultaneously

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That's true! But if we ever have to test these manually to verify, 30 seconds feels like an eternity

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm open to switching to backup codes in this PR, but my feeling is to get something out now so smoke tests can start again

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Makes sense, I agree, probably better to ship something sooner

@mitchellhenke mitchellhenke force-pushed the mitchellhenke/where-theres-smoke-tests branch 4 times, most recently from 1bbaeee to 5a0fd18 Compare October 27, 2021 19:48
@mitchellhenke mitchellhenke marked this pull request as ready for review October 27, 2021 20:00
Copy link
Copy Markdown
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines 294 to 295
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

just leaving a note to make sure that we undo this before mergin

Mitchell Henke and others added 5 commits October 27, 2021 15:52
Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
@mitchellhenke mitchellhenke force-pushed the mitchellhenke/where-theres-smoke-tests branch 2 times, most recently from 967026d to 4332b7e Compare October 27, 2021 21:11
@mitchellhenke mitchellhenke force-pushed the mitchellhenke/where-theres-smoke-tests branch from 4332b7e to 90436a5 Compare October 27, 2021 21:24
@mitchellhenke mitchellhenke merged commit 9f8aa76 into main Oct 27, 2021
@mitchellhenke mitchellhenke deleted the mitchellhenke/where-theres-smoke-tests branch October 27, 2021 21:38
zachmargolis added a commit that referenced this pull request Oct 28, 2021
* smoke testing test

* Update spec/support/monitor/monitor_email_helper.rb

Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>

* lint

* fix

* remove old environment variable usage

* remove gmail

* re-enable smoke test alerts

Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
(cherry picked from commit 9f8aa76)
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