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

Add read only #65

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Add read only #65

wants to merge 3 commits into from

Conversation

dustinblack
Copy link
Member

Changes introduced with this PR

Please explain your changes here.


By contributing to this repository, I agree to the contribution guidelines.

@dustinblack
Copy link
Member Author

There is currently a problem with this because I need to only set the command line parameter if and only if a value, whether true or false is passed to the parameter, and otherwise I do not want to include this parameter at all. The schema boolean type does not allow the default value to be set to nil, so when setting it to false I end up getting the parameter.

This is important because it overrides a global default, and that global default may be set to true or false, and we don't want to override the default unless we know we are doing it on purpose. If the global default on the system is set to true and our deployer defaults to false then we will override the global default without knowing it.

@dbutenhof
Copy link

This is important because it overrides a global default, and that global default may be set to true or false

In other words we need an Optional[bool], essentially, for the deployer option. We can't do that? 😦

"ReadonlyRootfs": schema.NewPropertySchema(
schema.NewBoolSchema(),
schema.NewDisplayValue(schema.PointerTo("ReadonlyRootfs"), schema.PointerTo("Execute container process with or without a read only root file system"), nil),
false,

Choose a reason for hiding this comment

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

Ah, yes. Good. We should have unit tests proving that the deployer module accepts true/false/missing ... I don't think the deployer tests have done all that much to prove that options passed in actually do what's intended (e.g., selinux labeling), but proving that we can/can't write to the root filesystem would be great.

Copy link
Contributor

Choose a reason for hiding this comment

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

Proving that we can/can't write to the root filesystem sounds like a functional test...but with a suitable mock, we could have unit tests for this stuff which would be great!

Copy link
Contributor

@webbnh webbnh left a comment

Choose a reason for hiding this comment

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

Looks good. Should we be implementing tri-state logic for any of the other options?

connector.go Show resolved Hide resolved
Comment on lines 7 to +10
SetContainerName(name string) ArgsBuilder
SetNetworkMode(networkMode string) ArgsBuilder
SetPrivileged(privileged bool) ArgsBuilder
SetReadOnlyRoot(readOnlyRootfs *bool) ArgsBuilder
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there any other options for which we should be implementing tri-state logic? (Like all of them? 😇)

(Also, the handling of -e looks...incomplete -- it seems like it silently ignores cases with no = as well as cases where the value of the environment variable contains a =....)

"ReadonlyRootfs": schema.NewPropertySchema(
schema.NewBoolSchema(),
schema.NewDisplayValue(schema.PointerTo("ReadonlyRootfs"), schema.PointerTo("Execute container process with or without a read only root file system"), nil),
false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Proving that we can/can't write to the root filesystem sounds like a functional test...but with a suitable mock, we could have unit tests for this stuff which would be great!

Comment on lines +345 to +346
schema.NewDisplayValue(schema.PointerTo("ReadonlyRootfs"), schema.PointerTo("Execute container process with or without a read only root file system"), nil),
false,
Copy link
Contributor

Choose a reason for hiding this comment

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

At least as far as the Podman man page is concerned, "read-only" is hyphenated.

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.

3 participants