Skip to content

Expose OracleContainer image name property as public constant - #1904

Closed
vpavic wants to merge 1 commit into
testcontainers:masterfrom
vpavic:improve-oracle
Closed

Expose OracleContainer image name property as public constant#1904
vpavic wants to merge 1 commit into
testcontainers:masterfrom
vpavic:improve-oracle

Conversation

@vpavic

@vpavic vpavic commented Sep 23, 2019

Copy link
Copy Markdown
Contributor

This makes it easier and safer for user code to refer to image name property. For example:

@BeforeAll
static void setUpClass() {
    Assumptions.assumeTrue(
        TestcontainersConfiguration.getInstance().getProperties().containsKey(OracleContainer.IMAGE_NAME_PROPERTY));
}

@rnorth

rnorth commented Sep 24, 2019

Copy link
Copy Markdown
Member

Thanks @vpavic - I think this is pretty sensible. I just wonder if there's a more general solution that we could look at.

#1345 will remove the image pull from the container constructor. This should make it safe to instantiate an OracleContainer even if the image is not available.

We would then have freedom to add some methods to the container class itself - for example, isImageAvailable(). Maybe with an optional boolean argument to prevent pulling of the image, if that's desired.

You could then make your assumption be: assumeTrue(myOracleContainer.isImageAvailable()), reducing the amount of 'knowledge' that's needed to find out whether the image is available.

WDYT?

@kiview

kiview commented Sep 24, 2019

Copy link
Copy Markdown
Member

@rnorth Maybe I am missing something, but I thought this is not the point of @vpavic.

I thought about a use case like:

if (!TestcontainersConfiguration.getInstance().getProperties()
  .containsKey(OracleContainer.IMAGE_NAME_PROPERTY)) {

  TestcontainersConfiguration.getInstance()
    .updateGlobalConfig(OracleContainer.IMAGE_NAME_PROPERTY, "myTestingImage");
}

Not sure though? 😅

@vpavic

vpavic commented Sep 24, 2019

Copy link
Copy Markdown
Contributor Author

Thanks for the feedback @rnorth.

Yes, @kiview is right - my idea was simply to support a safer way of verifying presence of oracle.container.image Testcontainers property.

The background is that in Spring Session we have an Oracle integration test that is conditional on presence of this property. This is needed due to lack of public Docker images for Oracle database so the test only runs when explicitly enabled.

@bsideup

bsideup commented Sep 24, 2019

Copy link
Copy Markdown
Member

@kiview the code you linked should never be used, actually :D
updateGlobalConfig is semi-internal API, but we had to call it from another package. Setting environment properties imperatively is not a good idea, since it will be shared between projects

@bsideup bsideup left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I agree with @rnorth that it would be better to not rely on internal details of the container (property) but on the behaviour, with maybe a helper method.

For now, one can attempt to instantiate a container and catch the exception

@vpavic

vpavic commented Sep 24, 2019

Copy link
Copy Markdown
Contributor Author

@bsideup Note that instantiating container is exactly what I prevent in this case, hence the use of this check in @BeforeAll.

@vpavic

vpavic commented Oct 3, 2019

Copy link
Copy Markdown
Contributor Author

Any further feedback on this? To clarify a bit more, as can be seen from the example in the opening comment, I'm preventing the entire test class from being set up, and would like to do it in a less fragile way, referencing the constant (or possibly something else, but it has to be static).

So any helper method would need to be static to be an option for this use case.

@stale

stale Bot commented Feb 27, 2020

Copy link
Copy Markdown

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you believe this is a mistake, please reply to this comment to keep it open. If there isn't one already, a PR to fix or at least reproduce the problem in a test case will always help us get back on track to tackle this.

@stale stale Bot added the stale label Feb 27, 2020
@stale

stale Bot commented Mar 12, 2020

Copy link
Copy Markdown

This issue has been automatically closed due to inactivity. We apologise if this is still an active problem for you, and would ask you to re-open the issue if this is the case.

@stale stale Bot closed this Mar 12, 2020
@vpavic

vpavic commented Aug 10, 2020

Copy link
Copy Markdown
Contributor Author

Could this be reopened please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants