-
Notifications
You must be signed in to change notification settings - Fork 1.1k
drop loading image when its already present #1763
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
Conversation
|
|
||
| public static void loadWiremock(K3sContainer container) { | ||
| Commons.load(container, WIREMOCK_TAR, WIREMOCK, wiremockVersion()); | ||
| if (!imageAlreadyInK3s(container, WIREMOCK_TAR)) { |
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.
we did this check in a previous PR for busybox, let's do it for every other 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.
Why not just move the check to Commons.load instead?
| .get(0) | ||
| .setImage(imageFromDeployment + ":" + pomVersion()); | ||
| } | ||
| else { |
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 don't recall the history of why we needed this, but we do not anymore. At this point in time the images are already un-tared/downloaded and loaded in k3s, as such we can drop this code
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 this was needed for Jenkins because on our end we hit threshold limits from Dockerhub inside of K3S. If we pull the images and load them in in the test iself we use our Dockerhub credentials and we don't run into the threshold limits from Dockerhub
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 I get it, will close this PR cause its a lot easier to make a new one then re-work this one. thx!
| .get(0) | ||
| .setImage(imageFromDeployment + ":" + pomVersion()); | ||
| } | ||
| else { |
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.
same comment as above
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.
same comment as above ;)
|
@ryanjbaxter ongoing changes to refactor integration tests. I'm still far from completing it, but its in progress |
|
Got it, when its ready ping me and I will give it a deeper look |
|
I mean, this PR is part of ongoing changes... should have been more explicit, sorry. This one is ready, but more will come, that was my point |
No description provided.