Smtp affair

[cstns]

Description

separating pr
already reviewed in: #4045

Related Issue(s)

Checklist

• I have read the contribution guidelines
• Suitable unit/system level tests have been added and they pass
• Documentation has been updated
  • Upgrade instructions
  • Configuration details
  • Concepts
• Changes flowforge.yml?
  • Issue/PR raised on FlowFuse/helm to update ConfigMap Template
  • Issue/PR raised on FlowFuse/CloudProject to update values for Staging/Production

Labels

• Includes a DB migration? -> add the area:migration label

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.72%. Comparing base (df2b0f1) to head (c4eb159).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4050   +/-   ##
=======================================
  Coverage   78.72%   78.72%           
=======================================
  Files         284      284           
  Lines       13009    13009           
  Branches     2897     2897           
=======================================
  Hits        10241    10241           
  Misses       2768     2768           

Flag	Coverage Δ
backend	78.72% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[cstns]

Need to find a solution to the e2e smtp child process failing on GH CI due to the process spawning successfully but is still pulling the smtp server container. This is not happening locally because the container is already available.

The forge app boots up in the meantime and fails to verify an email connection, sets global config to email disabled causing issues with e2e tests.

e2e logs:

 [STARTED] Task without title.
/usr/local/bin/npm run cy:web-server:os

> @flowfuse/flowfuse@2.5.0 cy:web-server:os
> node ./test/e2e/frontend/test_environment_os

Mailpit: Starting e-mail server...
Mailpit: Web UI available at http://localhost:8025/ with SMTP listening on port 1025
Mailpit: Unable to find image 'axllent/mailpit:latest' locally

[SUCCESS] Task without title.

Opening Cypress...
Mailpit: latest: Pulling from axllent/mailpit

[2024-06-20T17:03:58.141Z] ERROR: Failed to verify email connection: Error: connect ECONNREFUSED 127.0.0.1:1025

DevTools listening on ws://127.0.0.1:32915/devtools/browser/0c4a2cb1-9f61-4cb6-a50e-89a3c51ae827
Mailpit: d25f557d7f31: Already exists
ee12d611a4de: Pulling fs layer
d4b283bca91a: Pulling fs layer
d4b283bca91a: Verifying Checksum
d4b283bca91a: Download complete
ee12d611a4de: Verifying Checksum
ee12d611a4de: Download complete
ee12d611a4de: Pull complete
d4b283bca91a: Pull complete
Digest: sha256:d1402e182df9a21a97441f545e01a1290290b0ee68f8ae3e595017b9a162348c
Status: Downloaded newer image for axllent/mailpit:latest

My current and best solution so far is to wait for the smtp server stdout message.

dockerProcess.stdout.on('data', (data) => {
    if (data.toString().includes('accessible via http://localhost')) {
        resolve(dockerProcess)
    }
})

The alternative would be what @hardillb & @ppawlowski proposed to run the smtp server as a separate CI service, but that would mean making changes to the npm scripts to create separate CI scripts that do not run the smtp server.

What we'd need to take into account is the compute time it would take for both scenarios and how GH would bill them and take a decision based on that

@ppawlowski @hardillb, I'd love some input on this.

[ppawlowski]

Regarding GitHub Actions billing time - IMO it does not matter how you spin up the dependent service, the billing time will remain the same.
What does matter is that if you use service container approach, steps defined within a job will not be executed until a container with a SMTP server is ready. This approach removes the need to check the availability of particular services within testing scripts.

[cstns]

The service container approach would definitely solve the CI issue along with parallel runs but would complicate the run scripts locally.

Currently I'm spinning a smtp server per test environment due to cypress configuration inheritance. This results with two smtp servers spinning (1025/8025 for os e2e and 1026/8026 for ee e2e environments) when running the entire suite or a single dedicated one when running a single environment. This is being reflected in GH actions as well, a smtp server being spun up for each environment run.

From what i can tell after reading the docs is if the service container is created for each job on a runner so there will be a single smpt service for both e2e environments (which would be desired)

The ideal outcome:

• CI
  • single smtp server listening on ports 1025 for smtp & 8025 for http via service worker
  • the smtp server not starting via the cypress environment as a spawend process for each environment
• local dev
  • single smtp server would start as spawned process when
    • running both cypress environments via npm run cy:web-server
    • running single environment ee/os via npm run cy:web-server:ee or npm run cy:web-server:os

As we stand:

• we have the npm run cy:web-server running the cy:web-server:os and cy:web-server:ee commands in paralel which results in potential port overlap when running locally
• having the smtp server spun up part of the cypress environment would cause it to run on CI as well

Any pointers on how to address these would be welcomed. The ci part is straight forward, the local part is what I consider to be tricky because I don't want to add additional overhead when running tests locally (ex spinning the smtp server manually).

My best idea so far would be to double the cy:webserver commands and alter them for CI use where they don't spin up the smtp server but don't like the duplication part as we already have a lot of script entries

[cstns]

made some alterations to the code to run the smtp server as a service in CI runs

[joepavitt]

@hardillb @ppawlowski could one of you take a look at this please, at your earliest convenience. It's blocking a couple of other PRs

[hardillb]

Couple of questions to check I've got everything

[hardillb]

Just want to be clear on this

The env var is called NO_SMTP_SERVER and is set to true in the GH Action setup,

This if will evaluate if set to false?

[cstns]

I relied on the fact that this env var will not be set anywhere else but CI and didn't take into account what values might be passed in. You are right though, it will evaluate to true no matter what value you'd pass.

I'll make it more concise

[hardillb]

@joepavitt @cstns Approved, can be merged when suits the other PRs

[hardillb]

@joepavitt @cstns actually scratch that the UI tests have failed

[cstns]

@joepavitt @cstns actually scratch that the UI tests have failed

my bad, was not careful when updated the if statement. The tests failed due to a port clash because the smtp child process started