USHIFT-500: Remove ConfigFile, DataDir and Manifests fields off MicroShift config #1026
Conversation
|
@dgrisonnet @benluddy PTAL |
dinhxuanvu
force-pushed
the
config-update
branch
2 times, most recently
from
abf83a0 to
4236e6e
Compare
docs/howto_config.md
Outdated
There was a problem hiding this comment.
The manifest directories are described in the next section, can we drop the mention here? Is the config docs the first place a user will look to figure out where persistent cluster state is stored?
There was a problem hiding this comment.
So far this is the only place where all of config settings are mentioned. So I took the liberty to update it. I'm not sure if there is a more formal docs for all of these somewhere else.
|
/retitle USHIFT-500: Remove ConfigFile, DataDir and Manifests fields off MicroShift config |
openshift-ci
bot
changed the title
dinhxuanvu
force-pushed
the
config-update
branch
2 times, most recently
from
54552d6 to
f1a693a
Compare
|
/retest |
openshift-merge-robot
added
the
needs-rebase
dinhxuanvu
force-pushed
the
config-update
branch
from
f1a693a to
aaa0007
Compare
openshift-merge-robot
removed
the
needs-rebase
dinhxuanvu
force-pushed
the
config-update
branch
2 times, most recently
from
1f6ec50 to
26a517c
Compare
|
/approve /assign @fzdarsky This looks in line with the discussion on Vu's original PR. Are there any outstanding concerns about this particular change? |
openshift-ci
bot
added
the
approved
The three fields ConfigFile, DataDir and Manifests don't need to be user-configurable. Those values should be set to be default values at all time. As a result, those fields can be removed off microshift config to avoid supporting these user-facing configuration in the future. Signed-off-by: Vu Dinh <vudinh@outlook.com>
dinhxuanvu
force-pushed
the
config-update
branch
from
26a517c to
627261e
Compare
|
@dinhxuanvu: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
@dhellmann please take a look when you have the time. Thanks |
dhellmann
left a comment
dhellmann
left a comment
There was a problem hiding this comment.
I think we can point to a few cleanups, but let's get this in now and iterate on those separately.
/lgtm
| func RunMicroshift(cfg *config.MicroshiftConfig, flags *pflag.FlagSet) error { | ||
|
|
||
| if err := cfg.ReadAndValidate(flags); err != nil { | ||
| if err := cfg.ReadAndValidate("", flags); err != nil { |
There was a problem hiding this comment.
I think instead of having ReadAndValidatate() be responsible for setting a default, this higher level function should do it. We can fix that in a follow-up PR, though.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: benluddy, dhellmann, dinhxuanvu The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest-required |
|
I realized after approving that the sample file (https://github.com/openshift/microshift/blob/main/packaging/microshift/config.yaml) also mentions some of these options. That should be a quick follow-up PR, too. |
|
@dhellmann @dinhxuanvu I took the liberty of opening https://issues.redhat.com/browse/USHIFT-528 to keep track of the requested follow-up changes. Please add anything I missed. |
| if len(c.ConfigFile) == 0 { | ||
| return nil | ||
| } |
6 participants
The three fields ConfigFile, DataDir and Manifests don't need to be user-configurable. Those values should be set to be default values at all time. As a result, those fields can be removed off microshift config to avoid supporting these user-facing configuration in the future.
Signed-off-by: Vu Dinh vudinh@outlook.com
Which issue(s) this PR addresses:
Closes #