-
Notifications
You must be signed in to change notification settings - Fork 11
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
Add JSON formatting check to the add-ign function #189
Conversation
@skyblaster if you can fix the test failure, ill review and merge. |
@baude I'm unfamiliar with these particular tests, and not sure why it's failing, so will have to defer to @ashley-cui If I download libhvee-0.6.0 or later releases, and run |
With a little debugging, I can say that the following URL needs to be updated as it results in a 404 which causes the win_test error: libhvee/test/e2e/fedora_test.go Line 18 in 24e21b4
Unfortunately it's not as simple as changing 39 to 40 or 41, as the new VHD images are compressed. Example from 39: https://kojipkgs.fedoraproject.org/compose/metadata-archive/2024/Fedora-Cloud-39-20241126.0/images.json
New compressed format: https://kojipkgs.fedoraproject.org/compose/cloud/latest-Fedora-Cloud-41/compose/metadata/images.json
|
@baude I took a stab at fixing the e2e testing and it now passes. I'm not sure why it appears to copy the decompressed image twice, so please have a look https://cirrus-ci.com/task/6376975813574656 Sorry for the lack of a sign-off. Please let me know if I should rebase, or resubmit the PR. |
@skyblaster thanks! I was working on this before PTO and same thing as you ... only difference is I added a command line tool to the test image to unxz because the golang library is known to be very slow for decompression. Lets go ahead and merge this and I can switch over to the command line once I'm caught back up |
) | ||
|
||
const ( | ||
fedoraBaseDirEndpoint = "https://kojipkgs.fedoraproject.org/compose/cloud/latest-Fedora-Cloud-39/compose" | ||
fedoraBaseDirEndpoint = "https://kojipkgs.fedoraproject.org/compose/cloud/latest-Fedora-Cloud-40/compose" |
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.
mind changing to 41?
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.
Just noticed this as well in my image update Pr and tried to do a quick fix #197
Anyway it is complicated as there is no f41 on the server: i.e. 404 https://kojipkgs.fedoraproject.org/compose/cloud/latest-Fedora-Cloud-41/
I asked in the Fedora cloud matrix to see if this is a bug or not on their end. So Sticking to f40 seems to be the right call for now
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.
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.
works for me
defer outFile.Close() | ||
|
||
_, err = io.Copy(outFile, r) | ||
if err != nil { |
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.
just return err
if err != nil { | ||
return "", err | ||
} | ||
return vhdPath, nil |
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.
return vhdPath, err
wdyt ?
@skyblaster Can you sign-off your commits, see https://github.com/containers/common/blob/main/CONTRIBUTING.md#sign-your-prs We cannot accept contributions with unclear copyright status. We can fixup the minor nits and migrate to the cli tool at a later point. In the interest of working CI I think this is great to merge |
Signed-off-by: Jason Berry <[email protected]>
Signed-off-by: Jason Berry <[email protected]>
My apologies for the delay and for not creating a branch of my original fork, so as to keep the two commits separate. Cheers! |
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.
LGTM
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Luap99, skyblaster The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Thanks for the work @skyblaster, no need to apologize |
@skyblaster thank you very much for your contribution! /lgtm |
Fixes #187