Switch all rootful to rootfull #13964
Conversation
We are inconsistent on the name, we should stick with rootfull. [NO NEW TESTS NEEDED] Existing tests should handle this and no tests for machines exists yet. Signed-off-by: Daniel J Walsh <[email protected]>
openshift-ci
bot
added
the
approved
|
Will this cause any kind of unhappiness or lost expectations for customers if they upgrade from 4.0 to 4.1? Especially RHEL? |
|
I'll send Gabriella a heads up for the RHEL doc side of things. |
| flags.BoolVar(&initOpts.Rootful, rootfulFlagName, false, "Whether this machine should prefer rootful container execution") | ||
| rootfullFlagName := "rootfull" | ||
| flags.BoolVar(&initOpts.Rootfull, rootfullFlagName, false, "Whether this machine should prefer rootfull container execution") | ||
| flags.SetNormalizeFunc(aliasFlags) |
There was a problem hiding this comment.
Should we still support rootful? Similar to how we're still supporting the --pull-always option under the covers?
There was a problem hiding this comment.
We do, that is what the SetNormalizeFunc does, It basically aliases rootful->rootfull, so if people built scripts with the old option, it will continue to work.
|
Since rootful->rootfull alias exists this should not cause any breakage. |
|
@containers/podman-maintainers PTAL |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: flouthoc, giuseppe, rhatdan 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 |
|
I'd disagree with this change, as in English the suffix is -ful: https://www.oxfordlearnersdictionaries.com/us/definition/english/ful Using
The list of words ending in -ful also is far longer than the list of words ending in -full. Another option would be to drop the suffix entirely; "root connections", "root users", "root storage" seem to make as much sense as "rootful" |
|
Sure both are used, but the default for most of the code was to use rootfull and most of the documentation, blogs etc. The goal is to have them consistent everywhere. rootfull Looks better to me visually. |
|
@rhatdan This also breaks existing podman machines, since we're writing "rootful" to a config file. I think we need to at least keep this. |
|
Let's try and get this wrapped up before RC1. Which is now prooobably on monday? |
|
@ashley-cui --rootful is still supported. So it will not break anything |
|
I think the problem is that the option is written to the config file not the cli option. So we break old installs because the field is no longer correctly unmarshalled! @ashley-cui Please open a PR to fix this. We should not break machine users. |
github-actions
bot
added
the
locked - please file new issue/PR
We are inconsistent on the name, we should stick with rootfull.
[NO NEW TESTS NEEDED] Existing tests should handle this and no tests for
machines exists yet.
Signed-off-by: Daniel J Walsh [email protected]