-
Notifications
You must be signed in to change notification settings - Fork 13
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
probe #74
base: main
Are you sure you want to change the base?
probe #74
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,39 @@ | ||||||
package provider | ||||||
|
||||||
// supportedOptions contains a mappnig for supported systemd options. If an option | ||||||
// is supported the key name will be returned. Unsupported either return an | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
// empty string (really not supported) or an alternative option that's better | ||||||
// than nothing at all. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's unclear how it is better than nothing at all. Is the value another option that we know is supported by older systemd versions? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd also add that the actual probing doesn't happen here but, eventually, when |
||||||
var supportedOptions = map[string]string{ | ||||||
"BindReadOnlyPaths": "BindReadOnlyPaths", | ||||||
} | ||||||
|
||||||
// ProbeSupportedOptions checks it the options in SupportedOptions are | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
// supported by the systemd version running on this system. It will emit Info | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd say warning logs. |
||||||
// logs for each unsupported option. | ||||||
func ProbeSupportedOptions() { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doesn't need to be exported. Or may it does as this is moved into the unit package where other unit stuff is? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or may it does as this is moved into the |
||||||
for option := range supportedOptions { | ||||||
ok := probe(option) | ||||||
switch option { | ||||||
case "BindReadOnlyPaths": | ||||||
if !ok { | ||||||
supportedOptions[option] = "BindPaths" // drop the RO bit | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This switch may grow a bit. Why not have another map with the alternatives? Eg alternativeOptions := map[string]string{
"BindReadOnlyPaths": "BindPaths",
}
for option := range supportedOptions {
if !probe(option) {
supportedOptions[option] = alternativeOptions[option]
}
} |
||||||
} | ||||||
} | ||||||
} | ||||||
} | ||||||
|
||||||
// probe probes system to see if option is supported. | ||||||
func probe(option string) bool { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this is something you are proposing to have merged without the actual probing (otherwise, it would be a draft PR, right?), please, state very clearly that this is a fake probe, as nothing is actually probed. |
||||||
return true | ||||||
} | ||||||
|
||||||
// Option return the option that is supported by the detected systemd. | ||||||
func Option(option string) string { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doesn't need to be exported. Or may it does as this is moved into the unit package where other unit stuff is? |
||||||
opt, ok := supportedOptions[option] | ||||||
if !ok { | ||||||
// not found in map, return option as-is | ||||||
return option | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given we know exactly what options are used in systemk, |
||||||
} | ||||||
return opt | ||||||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -84,7 +84,11 @@ func (u *File) String() string { | |||||
} | ||||||
|
||||||
// Insert adds name=value to section and returns a newly parsed pointer to File. | ||||||
// If name is the empty string this is a noop. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
func (u *File) Insert(section, name string, value ...string) *File { | ||||||
if name == "" { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please, add a comment above stating that this is empty only when the option is not supported by the current systemd and no alternative is available. |
||||||
return u | ||||||
} | ||||||
opts := make([]*unit.UnitOption, len(value)) | ||||||
for i := range opts { | ||||||
opts[i] = &unit.UnitOption{ | ||||||
|
@@ -98,7 +102,11 @@ func (u *File) Insert(section, name string, value ...string) *File { | |||||
} | ||||||
|
||||||
// Overwrite overwrites name=value in the section and returns a new File. | ||||||
// If name is the empty string this is a noop. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
func (u *File) Overwrite(section, name string, value ...string) *File { | ||||||
if name == "" { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please, add a comment above stating that this is empty only when the option is not supported by the current systemd and no alternative is available. |
||||||
return u | ||||||
} | ||||||
opts := make([]*unit.UnitOption, len(u.Options)) | ||||||
j := 0 | ||||||
for _, o := range u.Options { | ||||||
|
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.