added in tests for bad checksums on ubuntu images, and added in some … #1481
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
khushboo-rancher
requested changes
Aug 27, 2024
There was a problem hiding this comment.
In general, LGTM. Couple of minor comments.
Can we edit the image created with bad checksum and provide the right one? If so, can we have that test to ensure volume can be crated with edited image. (Need not be in the same PR, you can create a separate ticket too. Either ways will work)
Have you run the impacted test cases to ensure they are running fine with the changes?
noahgildersleeve
force-pushed
the
image-url-checksum
branch
from
7e96195
to
55b6eed
Compare
lanfon72
reviewed
Aug 28, 2024
albinsun
reviewed
Aug 29, 2024
noahgildersleeve
force-pushed
the
image-url-checksum
branch
6 times, most recently
from
97859b3
to
59a2fe8
Compare
noahgildersleeve
force-pushed
the
image-url-checksum
branch
from
59a2fe8
to
92759c7
Compare
albinsun
requested changes
Oct 7, 2024
…pydoc docs in the images and volumes sections added in opensuse checksum and added associated config data fixed pep8 errors made some requested changes from PR review PR review changes removed old status check fixed an improper assertion, added a default for image_checksum, and added a setter for opensuse_checksum changed assertion message to be more clear changed checksum default type removed default checksums from default fixed some pep8 errors fixed some errors and made some adjustments based on PR review comments added default value for image_checksum
noahgildersleeve
force-pushed
the
image-url-checksum
branch
from
92759c7
to
301b22a
Compare
lanfon72
approved these changes
Oct 10, 2024
There was a problem hiding this comment.
In general LGTM,
IMO, refactor should be done in another PR, when adding new test case, we should not do any refactor.
and I saw that we add fixture ubuntu_checksum and opensuse_checksum but they will not be used in anywhere, if it is not used, we should not add them.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…pydoc docs in the images and volumes sections
added in opensuse checksum and added associated config data
fixed pep8 errors
Which issue(s) this PR fixes:
Issue #1121
What this PR does / why we need it:
This adds image checksum testing to the automation
Special notes for your reviewer:
I added in some docs on other tests in the images and volumes scenarios. Also this will allow command line adding of the checksum for the images. This will also work if you just remove the text in the checksum and leave it blank, and the negative test case will still run correctly as well.
Additional documentation or context
I checked that the other tests weren't being affected and I don't think any of them should be.