-
Notifications
You must be signed in to change notification settings - Fork 54
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
blueprint: fix cacerts name for TOML #1076
Conversation
I don't think this can be changed, any existing blueprint using this field will not be parsed correctly. |
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.
Except for the leftover debug prints, this looks OK to me. 👍
This has not yet been exposed to users in any form AFAICT, and the TOML parsing here is not the same as the one used on-prem in |
So removed the logging line and added a test, but I am not sure where exactly this shell test is being executed. |
Tests look good, why this isn’t getting merged? Not sure if I should rebase. |
You dismissed my review by force-pushing. If you need just to rebase, using GH UI does not dismiss approvals. |
Ah now I remember, I was on the wrong branch and accidentally pushed here instead, sorry! Cheers. |
exit 1 | ||
fi | ||
} | ||
|
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.
Out of curiousity, where do I see this output? I am unable to locate this job.
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.
If the manifest were not rebuilt during the CI run (because it didn't change), you'd find a link to the run in which it was tested in the job that generated it (e.g. https://gitlab.com/redhat/services/products/image-builder/ci/images/-/jobs/8565731980). Since the manifest didn't change and you added the test, it was not run at all. A complete manifest regeneration would be required. That can be achieved by bumping the rngseed
in
Line 3 in 048c8c6
"rngseed": 1, |
I did it in #1093 and if my comment about the potential typo from above is correct, it should fail 🤔
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 story continuous:
- osbuild: new stage 'cacert' (HMS-4839) #907 added the
cacert
customization only toall-customizations.json
, all-customizations.json
is applied only when buildingqcow2
Lines 2 to 13 in 048c8c6
"./configs/all-customizations.json": { "distros": [ "rhel-10*", "rhel-9*", "rhel-8*", "centos*", "fedora*" ], "image-types": [ "qcow2" ] }, - We do not boot-test
qcow2
in this repository CI, so the change in thebase-host-check.sh
won't run in the CI.
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.
Yeah, apologies: #1096
fi | ||
|
||
echo "📗 Checking extracted CA cert file" | ||
if ! [ -e "/etc/pki/ca-trust/source/extracted/pem/directory-hash/Test_CA_for_osbuild.pem.pem" ]; then |
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.
@lzap I just noticed a potential typo in the filename - note the double .pem.pem
🤔
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.
Yeah, this is curious, do we generate funky filenames here or is the test not running correctly?
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.
@mvo5 see #1076 (comment). The cacert
customization is not applied to any image we boot-test, so this code is never executed.
This is to test the new check added by PR osbuild#1076 [0]. [0] osbuild#1076 Signed-off-by: Tomáš Hozza <[email protected]>
@avitova found some issues, I am going to address them.
To test the CA generation run this:
It creates the following file in
test/data/manifests
:I think it works.