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

feat(ci): Add e2e tests against latest docker images #22576

Open
wants to merge 35 commits into
base: main
Choose a base branch
from

Conversation

alexvy86
Copy link
Contributor

@alexvy86 alexvy86 commented Sep 19, 2024

Description

Adds a new stage for e2e tests against a local routerlicious environment spun up with docker in the build agent. This exercises our e2e tests against the current version (head of main) of all server code (vs the tests we run against the internal routerlicious deployment in AKS, which runs not-terribly-old-but-never-head-of-main docker images of the server components), which should help get us some confidence that the current version of server isn't breaking something (which in fact already seems to have happened; keep reading).

For this, a few adjustments (and fixes?) were necessary in the docker setup, server configuration, and the e2e tests pipeline:

  • Add an nginx container to the docker environment spun up from server/docker-compose.yml, with the nginx configuration defined in the repo. This is necessary so recent changes that cause alfred to redirect certain requests to nexus are in place. Otherwise older FF versions do not work against this environment.
  • Split the image tag used for historian from the ones used for gitrest and gitssh. They come from different pipelines from different build numbers so with a single variable we would not be able to find all of them at the same time.
  • Enable whole summary upload by default in alfred. Without this, I was able to run things correctly using the latest routerlicious + gitrest + gitssh images, but when adding the latest historian image into the mix, I started seeing many errors in the historian logs, and the tests ran, but they did it way slower. I don't understand the interaction here, but we're moving away from shredded summary upload anyway, so changing the default in the docker images to be whole summary upload seems to make sense regardless.
    • In my opinion, this basically means that we (kind of?) broke the server environment provided by our docker images at some point. The tests still work, but something seems to not be working as intended, if one were to spin up a server environment with the latest docker images available internally. It has had no public impact because the image versions that are available publicly for historian, gitrest, and gitssh, are pretty old (and things seem to work fine with those, but they're far from being the latest).
  • Update the template for e2e test runs in the pipeline so it can take pre/post steps (post-steps capability was already there but unused), where the new stage can spin up and spin down the docker environment.

Reviewer Guidance

The review process is outlined on this wiki page.

This sample run (msft internal) was done with only the new stage. Runtimes are pretty good and no tests failed. And this run is more recent (18a1ae0) and with a more final version of this PR; all tests still passing.

@github-actions github-actions bot added area: build Build related issues area: server Server related issues (routerlicious) base: main PRs targeted against main branch labels Sep 19, 2024
@alexvy86
Copy link
Contributor Author

Server folks (@znewton , @pradeepvairamani, tag anyone else that could/should chime in), I'm interested in your thoughts on the changes to the docker compose files and the routerlicious config. I'm not familiar with any workflows you guys might have with these files, but I think everything here is safe and/or necessary for a fully functional local environment (including supporting older FF client versions).

Comment on lines +26 to +28
# THIS VOLUMES IS FOR LOCAL TESTING, IT SHOULD NOT GET MERGED
volumes:
- ./routerlicious/packages/routerlicious/config/config.json:/usr/src/server/packages/routerlicious/config/config.json
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Used this locally to validate running e2e tests against the latest server image, but with whole summary upload enabled (the image itself doesn't do it). This will go away before merging.

Comment on lines +89 to +90
# volumes:
# - ./routerlicious/packages/routerlicious/config/config.json:/home/node/server/packages/routerlicious/config.json
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will remove too, this was also for local testing.

Comment on lines +350 to +352
${{ if parameters.env }}:
env:
${{ parameters.env }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The new stage doesn't use it right now, and if nothing is passed this ends up being a syntax error with the current code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: build Build related issues area: server Server related issues (routerlicious) base: main PRs targeted against main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant