-
Notifications
You must be signed in to change notification settings - Fork 3
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
delete image if it exists #101
base: main
Are you sure you want to change the base?
Conversation
Have you tested with the image present on your machine and then running the test? |
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.
Have you tested with image present on your machine and then run the test?
@@ -39,11 +39,13 @@ | |||
* | |||
* @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") |
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.
it should be - "delete the docker image {word} if it exists on the device"
// This could be improved by using the API on a local host. | ||
|
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.
Remove the extra line if not needed
public void checkDockerImageIsMissing(String image) { | ||
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())) { |
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.
To check if the image is present or not, predicate should not be negated here.
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.
Sorry.. why? There is not logic change in here. Instead of error out the code. I just return boolean result?
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.
Earlier it was going to throw error of the image exists, but now you are returning true if it exists.
@@ -65,18 +67,30 @@ public void removeDockerImage(String image) { | |||
LOGGER.debug("Removed docker image {}: {}", image, result); | |||
} | |||
|
|||
private void checkDockerImagePresence(String image, Predicate<Set<String>> validity, String message) { | |||
private boolean isDockerImagePresence(String image, Predicate<Set<String>> validity) { |
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.
The name of this method needs to change. I suggest naming it "checkPresence" since it is taking in a predicate.
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.
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); |
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.
From the original ticket: Is there any risk to customer data? What if the customer has a Docker image installed with the same name?
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 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.
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.