Skip to content
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: add cacert customization #4487

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

lzap
Copy link
Contributor

@lzap lzap commented Nov 21, 2024

This is a follow-up to

https://github.com/osbuild/images/pull/907/files

I am shooting in the dark, not sure how can I test this locally. Let me know how can I test this.

@@ -0,0 +1,5 @@
package blueprint

type CACustomization struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this belongs in the customizations.go with the rest of the structs. Putting it into a separate file makes it harder to read the code.

@@ -0,0 +1,5 @@
package blueprint

type CACustomization struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This maybe out of scope for this PR but we have exactly this struct define in "images" and it is public, couldn't we just import it instead of copying it (the same question for most of the others but I guess this is why it's out of scope).

Copy link

@avitova avitova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few nitpicks I noticed.

@@ -31,6 +33,7 @@ type Customizations struct {
Installer *InstallerCustomization `json:"installer,omitempty" toml:"installer,omitempty"`
RPM *RPMCustomization `json:"rpm,omitempty" toml:"rpm,omitempty"`
RHSM *RHSMCustomization `json:"rhsm,omitempty" toml:"rhsm,omitempty"`
CACerts *CACustomization `json:"cacerts,omitempty" toml:"ca,omitempty"`
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is there a different field name for toml than for json?

Comment on lines +444 to +453
if c.CACerts != nil {
for _, bundle := range c.CACerts.PEMCerts {
_, err := cert.ParseCerts(bundle)
if err != nil {
return err
}
}
}

return nil
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking that maybe we could leave the happy path here? An early return, and then continue with parsing the values.

Comment on lines +81 to +91
case int64:
if s < 0 {
return 0, fmt.Errorf("cannot be negative")
}
return uint64(s), nil
case float64:
if s < 0 {
return 0, fmt.Errorf("cannot be negative")
}
// TODO: emit warning of possible truncation?
return uint64(s), nil
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
case int64:
if s < 0 {
return 0, fmt.Errorf("cannot be negative")
}
return uint64(s), nil
case float64:
if s < 0 {
return 0, fmt.Errorf("cannot be negative")
}
// TODO: emit warning of possible truncation?
return uint64(s), nil
case int64, float64:
if s < 0 {
return 0, fmt.Errorf("cannot be negative")
}
return uint64(s), nil

But I am not sure, what is the plan with this, as there is also the TODO comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants