-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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 utility class to allow image names to be configured … #277
Conversation
7bbff66
to
17e9eb6
Compare
public String vncRecordedContainerImage = "richnorth/vnc-recorder:latest"; | ||
|
||
// No public constructor - must use getInstance() method. | ||
private TestcontainersConfiguration() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can be replaced with NoArgsConstructor(access = lombok.AccessLevel.PRIVATE)
@@ -24,7 +25,7 @@ | |||
private final int servicePort; | |||
|
|||
public AmbassadorContainer(LinkableContainer otherContainer, String serviceName, int servicePort) { | |||
super("richnorth/ambassador:latest"); | |||
super(TestcontainersConfiguration.getInstance().ambassadorContainerImage); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's more conventional to use getters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right - I even put the @Data
annotation on for that reason! Whoops.
17e9eb6
to
015a32c
Compare
Squashed and pushed updates. |
try (final InputStream inputStream = configOverrides.openStream()) { | ||
properties.load(inputStream); | ||
|
||
config.ambassadorContainerImage = properties.getProperty("ambassador.container.image", config.ambassadorContainerImage); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realized that if file if present, it will override both properties. What if user will override ambassador.container.image
, but not vncrecorder
? We probably should add a check for it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be OK - this is why I provided the current value (default) as the 2nd, default, parameter to properties.getProperty(...)
. The intention is that if the user only provides one property, the other stays the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oooops, sorry, missed that!
015a32c
to
40ddc29
Compare
... via a classpath file
Replace static string names for ambassador and vnc recorder container images with classpath lookup.
Add note to readme re overriding hardcoded image names
Refs #259, #276