Skip to content

Conversation

jie0917
Copy link
Contributor

@jie0917 jie0917 commented Mar 11, 2022

Issue #, if available:
Remove test docker image from device, irrespective of its presence at the start of the test
Description of changes:

as ticket mentioned we should delete image
Why is this change necessary:

How was this change tested:
manually test in my local machine
Any additional information or context required to review the change:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@abanthiy
Copy link
Contributor

Have you tested with the image present on your machine and then running the test?

Copy link
Contributor

@abanthiy abanthiy left a comment

Choose a reason for hiding this comment

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

Have you tested with image present on your machine and then run the test?

* @param image fully qualified image name in <code>name:tag</code> representation
*/
@Given("the docker image {word} does not exist on the device")
@Given("deleted the docker image {word} if exists on the device")
Copy link
Contributor

Choose a reason for hiding this comment

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

it should be - "delete the docker image {word} if it exists on the device"

private void checkDockerImagePresence(String image, Predicate<Set<String>> validity, String message) {
private boolean isDockerImagePresence(String image, Predicate<Set<String>> validity) {
// This could be improved by using the API on a local host.

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove the extra line if not needed

Predicate<Set<String>> predicate = Set::isEmpty;
checkDockerImagePresence(image, predicate.negate(),
"The image " + image + " is already on the device. Please remove the image and try again.");
if (isDockerImagePresence(image, predicate.negate())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

To check if the image is present or not, predicate should not be negated 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.

Sorry.. why? There is not logic change in here. Instead of error out the code. I just return boolean result?

Copy link
Contributor

Choose a reason for hiding this comment

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

Earlier it was going to throw error of the image exists, but now you are returning true if it exists.

}

private void checkDockerImagePresence(String image, Predicate<Set<String>> validity, String message) {
private boolean isDockerImagePresence(String image, Predicate<Set<String>> validity) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The name of this method needs to change. I suggest naming it "checkPresence" since it is taking in a predicate.

Copy link
Contributor

@abanthiy abanthiy left a comment

Choose a reason for hiding this comment

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

Even though the check does not exist right now but make sure your commit message follows the convention specified here - https://www.conventionalcommits.org/en/v1.0.0/

checkDockerImagePresence(image, predicate.negate(),
"The image " + image + " is already on the device. Please remove the image and try again.");
if (isDockerImagePresence(image, predicate.negate())) {
removeDockerImage(image);
Copy link

Choose a reason for hiding this comment

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

From the original ticket: Is there any risk to customer data? What if the customer has a Docker image installed with the same name?

Copy link
Contributor

@abanthiy abanthiy left a comment

Choose a reason for hiding this comment

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

I think as a solution to the problem highlighted in the ticket, it is better to delete the image at the end of the test as opposed to removing it if it exists in the begining. That will be more like a cleanup.

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.

3 participants