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

Add option to override image validation #108

Merged

Conversation

joaodubas
Copy link
Contributor

@joaodubas joaodubas commented Jul 2, 2024

Expose the image validation process to the end user so that custom images can be used without re-implementing all the container specifications.

The image validation is controlled by a regular expression set in the check_image on the Testcontainers.Container module and accessible through the method with_check_image/2. All the specialized containers are adjusted to provide a default value for this check and allow an override.

For example, instead of using the redis image, it's possible to use the valkey alternative without major changes:

{:ok, _} = Testcontainers.start_link()
config = Testcontainers.RedisContainer.new() |> Testcontainers.RedisContainer.with_image("valkey/valkey:7.2.5-bookworm") |> Testcontainers.with_check_image(~r/.*\bvalkey\b.*/)
{:ok, container} = Testcontainers.start_container(config)

Solves issue #106.

Add methods to control when to perform image validation and define the
validation behavior on `Testcontainers.Container` module.

Also, adapt all containers to use these methods.
@jarlah
Copy link
Member

jarlah commented Jul 2, 2024

need to think about this. I am not sure if it's smart to continue with starts_with check on string.

Two suggestions:

  1. use a regex instead of supplying the image as a string. This could be made possible/userfriendly by making an "overloaded" versions of with_default_image that converts the binary input to a regex (or fails if it's not a valid regex). This way, you can supply both a string or a regex.
  2. assume that when default_image is not supplied there is no image validation. And replace check_image? with a check_image function, with an accompanying method with_check_image that checks if the parameter is a function that takes the appropriate parameters to be used in the validation of the image. Cutting out the need for the extra boolean field.

Let me sleep on it, and do make yourself some opinions on this also. I might not have the best ideas around, but I'm aiming for future proofing and avoiding quick fixes ;)

@joaodubas
Copy link
Contributor Author

I agree that adding two new fields to control the validations seems strange. We can pass an optional function that validates the image's compliance. Something along the lines of:

defmodule Testcontainers.Container do

  defstruct [..., check_image: fn _image -> true end]

  def with_check_image(%__MODULE__{} = config, check_image) when is_function(check_image) do
    %__MODULE__{config | check_image: check_image}
  end

  def check_image(%__MODULE__{} = config) do
    if config.check_image.(config.image) do
       {:ok, config}
    else
      {:error, "Unexpected image #{config.image}. If this is a valid image, provide a broader `check_image` function to the container configuration."}
    end
  end

I'll play on more generic solutions and seek inspiration on the other implementations of Testcontainers.

This is just a simple translation of the current validation. I will work
out a way to reduce the code duplication.
With this update, it's easier to match parts of the image repository and
assume any registry or tag.
@joaodubas joaodubas changed the title Add option to disable image validation Add option to override image validation Jul 3, 2024
@joaodubas
Copy link
Contributor Author

I re-implemented the validation using regular expressions; overall, it seems a more straightforward solution. But I don't know if we should implement a parser for the image and try to extract the register/user/image/tag data and use this information.

lib/container.ex Outdated Show resolved Hide resolved
With this, we let the end user know that the string can't be converted
to a valid regular expression.
Create `guard` clause to validate `check_image`.
Conflicts:
    lib/container.ex
Copy link
Member

@jarlah jarlah left a comment

Choose a reason for hiding this comment

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

Nice work. Thanks for the contribution. This is backwards compatible, so i will make a patch release with this change. The commits will be squashed when merged fyi, unless you rebase and squash the commits yourself.

lib/ecto.ex Show resolved Hide resolved
lib/ecto.ex Show resolved Hide resolved
@jarlah jarlah merged commit 04e6e2c into testcontainers:main Jul 6, 2024
6 checks passed
@joaodubas
Copy link
Contributor Author

Nice work. Thanks for the contribution. This is backwards compatible, so i will make a patch release with this change. The commits will be squashed when merged fyi, unless you rebase and squash the commits yourself.

Thanks for all the assistance and review.

@joaodubas joaodubas deleted the jpd/feat/disable-image-validation branch September 28, 2024 14:05
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