USHIFT-528: Mop up removal of manifests, configFile, and dataDir config fields. #1076
Conversation
benluddy
commented
Nov 1, 2022
benluddy
commented
- Removes references from sample config file.
- Remove defaulting behavior from ReadAndValidate.
- Don't try to load from file if no config file path is available.
- Removes references from sample config file. - Remove defaulting behavior from ReadAndValidate. - Don't try to load from file if no config file path is available.
openshift-ci
bot
added
the
approved
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: benluddy, 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 |
|
/cc @dhellmann |
|
/retest-required |
| } | ||
| if err := c.ReadFromConfigFile(configFile); err != nil { | ||
| return err | ||
| if configFile != "" { |
There was a problem hiding this comment.
Should an empty filename be an error now?
There was a problem hiding this comment.
It wasn't in the past -- that seems to have been an inadvertent change of the original PR.
There was a problem hiding this comment.
Maybe it makes sense to bring that back when we drop the environment variable stuff? Let's think about it and do it separately if we decide we want it.
There was a problem hiding this comment.
/hold
For this PR, are you proposing that it should be a fatal error if neither a user nor a global config file exists? This has historically been treated as though there were an existent-but-empty config file, and only became an error in #1026 (comment). My first reaction was to restore the original behavior since there had been no discussion/intent to change it.
There was a problem hiding this comment.
I was proposing that this function consider an empty string an invalid filename. If we're saying the file doesn't have to exist (that makes sense), and the caller might pass an empty string when there is no file to read, then this function could treat the empty string as not an error.
There was a problem hiding this comment.
As it is, MicroShift follows your latter logic. The configFile is set by the findConfigFile() func (L143), which will return an empty string if no file is found. The empty string value should continue to be interpreted as "use defaults" rather than throwing a fatal error.
There was a problem hiding this comment.
/unhold
The original behavior (i.e. to work without a config file) makes sense to me. I'm going to proceed with restoring it in this PR. If that behavior's not desirable, it can be changed deliberately sometime later.
| return err | ||
| } | ||
| } | ||
| if err := c.ReadFromEnv(); err != nil { |
There was a problem hiding this comment.
I think we could drop the environment variable stuff at some point, too. That was for configuring a container version, and we don't support deploying that way.
openshift-ci
bot
added
do-not-merge/hold
|
/retest-required |
|
/test e2e-openshift-conformance-sig-api-machinery |
|
@benluddy: all tests passed! 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. |
5 participants
