Skip to content
This repository has been archived by the owner on Apr 6, 2023. It is now read-only.

feat(settings): Make suggestion providers much more configurable at runtime #125

Merged
merged 4 commits into from Sep 20, 2021
Merged

feat(settings): Make suggestion providers much more configurable at runtime #125

merged 4 commits into from Sep 20, 2021

Conversation

mythmon
Copy link
Contributor

@mythmon mythmon commented Sep 9, 2021 •
edited

  • Remove Cows and lifetimes from providers and box all of them
  • feat(settings): Make suggestion providers much more configurable at runtime

fixes #120

BREAKING CHANGE: The configuration of providers is more free-form now, and the old settings will need updated.

@mythmon mythmon changed the title (settings): Make suggestion providers much more configurable at runtime feat(settings): Make suggestion providers much more configurable at runtime Sep 9, 2021
@mythmon
Copy link
Contributor Author

mythmon commented Sep 13, 2021

@hackebrot I made some changes to the test-engineering tests, since the configuration system changed enough that it wasn't compatible anymore.

@hackebrot
Copy link
Member

Thank you for letting me know, @mythmon! I'll take a look tomorrow morning. 📋

hackebrot
hackebrot reviewed Sep 14, 2021
View reviewed changes
Copy link
Member

@hackebrot hackebrot left a comment

Choose a reason for hiding this comment

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

Hi @mythmon! 👋🏻 I added my comments inline.

test-engineering/README.md

```text
docker compose -f test-engineering/docker-compose.yml up --abort-on-container-exit --build
docker-compose -f test-engineering/docker-compose.yml up --abort-on-container-exit --build
Copy link
Member

Choose a reason for hiding this comment

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

Newer versions of docker come with a compose subcommand. Probably makes sense to refer to the docker-compose script for compatibility here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, interesting. My version of docker does not have a compose subcommand, and this is a typo I make very often, so I assumed it was the same.

merino/src/docs/testing.rs
//! Merino's APIs using more opaque techniques. They cannot configure the server
//! per test, and are more concerned with external contracts and behavior.
//!
//! For more details see the README.md file in the `test-engineering` directory.
Copy link
Member

Choose a reason for hiding this comment

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

I didn't know about this docs page! This is super helpful. Thank you for documenting the new tests here. 📝

merino/src/docs/testing.rs
//! There are two major testing strategies used in this repository: unit tests,
//! and integration tests.
//! There are three major testing strategies used in this repository: unit tests,
//! Rust integration tests, and Python system tests.
Copy link
Member

Choose a reason for hiding this comment

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

I'd probably not mention that the system tests are written in Python since it's not relevant to the coverage that these tests provide. What's more important is that a HTTP client running in a Docker container sends HTTP requests to Merino running in a Docker container (which is closer to how Merino runs in prod that compared to unit or the Rust integration tests).

test-engineering/docker-compose.yml
@@ -3,26 +3,11 @@ services:
merino:
# See `docker-image-build` job in `.circleci/config.yml`
image: app:build
build: ..
Copy link
Member

Choose a reason for hiding this comment

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

Good idea! 👍🏻

merino/src/docs/testing.rs
@@ -51,3 +55,11 @@
//!
//! For more details, see the documentation of the `merino-integration-tests`
//! crate.
//!
//! ## System tests
Copy link
Member

Choose a reason for hiding this comment

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

I suggest we also rename the CircleCI job to reflect this definition! 🤖

@hackebrot
Copy link
Member

Looks like the system tests failed on CI with a timeout connecting to Merino:

merino_client | wait-for-it.sh: timeout occurred after waiting 15 seconds for merino:8000

For some reason it still ran the pytest tests after that. 🤔

@mythmon
Copy link
Contributor Author

mythmon commented Sep 14, 2021 •
edited

Looks like the system tests failed on CI with a timeout connecting to Merino:

By default wait-for-it.sh will still run the command if it times out. This is similar to something I had hit locally. I can work on this, though if you'd prefer to make changes yourself, please go ahead. I'll be at RustConf today, so won't get a chance to look at this right away.

@hackebrot
Copy link
Member

I was looking into the timeout issues again today. The log message below suggests that Merino is running on a port 8080 even though config/ci.yaml sets it to 8000:

config:

http:
  # The host and port Merino listens on
  listen: "0.0.0.0:8000"

log:

INFOactix_server::builder: Starting "actix-web-service-0.0.0.0:8080" service on 0.0.0.0:8080

@mythmon
Copy link
Contributor Author

mythmon commented Sep 15, 2021

Well that's not good. I'll look into that and see what's going on.

@mythmon mythmon requested a review from a team September 16, 2021 15:39
@mythmon mythmon marked this pull request as ready for review September 16, 2021 15:39
mythmon
mythmon commented Sep 16, 2021
View reviewed changes
Dockerfile
@@ -79,10 +79,6 @@ COPY --from=builder /app/bin /app/bin
COPY --from=builder /app/version.json /app
COPY --from=builder /app/config /app/config

ARG HOST=0.0.0.0
ARG PORT=8080
ENV MERINO_HTTP__LISTEN="${HOST}:${PORT}"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was the real fix. The rest of the changes in this commit are useful things I figured out while debugging this.

@hackebrot
Copy link
Member

The integration tests job failed with the following error:

merino_client | pytest: error: unrecognized arguments: merino:8000 -- pytest -vv

Looks like something is wrong with the command for the merino_client Docker service and how /wait-for-it.sh forwards arguments. 🤔

@mythmon
Copy link
Contributor Author

mythmon commented Sep 20, 2021

@hackebrot btw, every commit of the form area: description goes into the changelog. For incremental fixes to a branch, I don't think we should use them. I like JR's pattern of denoting these fixup-commits that don't contribute to the changelog (and should be squashed or considered squashed) by writing f description as in "fixup".

@mythmon
Copy link
Contributor Author

mythmon commented Sep 20, 2021

Rebased to fix merge conflicts

@mythmon
Copy link
Contributor Author

mythmon commented Sep 20, 2021

@mozilla-services/context-services This should be ready for review now that @hackebrot fixed up the tests.

@mythmon mythmon merged commit 219e1f7 into main Sep 20, 2021
@mythmon mythmon deleted the the-full-webpack branch September 20, 2021 18:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@hackebrot hackebrot hackebrot left review comments

@pjenvey pjenvey pjenvey approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow full configuration of Merino providers
3 participants
@mythmon @hackebrot @pjenvey