Skip to content

USHIFT-528: Mop up removal of manifests, configFile, and dataDir config fields. #1076

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.

Sign up for GitHub

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

Merged
openshift-merge-robot merged 1 commit into from
Nov 2, 2022
Merged

USHIFT-528: Mop up removal of manifests, configFile, and dataDir config fields. #1076

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 0 additions & 8 deletions packaging/microshift/config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,17 +22,9 @@ cluster:
# MTU for CNI
#mtu: "1400"

# Location for data created by MicroShift
#dataDir: /var/lib/microshift

# Log verbosity (0-5)
#logVLevel: 0

# Locations to scan for manifests to load on startup
#manifests:
#- /usr/lib/microshift/manifests
#- /etc/microshift/manifests

# The IP of the node (defaults to IP of default route)
#nodeIP: ""

Expand Down
4 changes: 2 additions & 2 deletions pkg/cmd/run.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,8 @@ func NewRunMicroshiftCommand() *cobra.Command {
}

func RunMicroshift(cfg *config.MicroshiftConfig, flags *pflag.FlagSet) error {
if err := cfg.ReadAndValidate("", flags); err != nil {
klog.Fatalf("Error in reading and validating flags", err)
if err := cfg.ReadAndValidate(config.GetConfigFile(), flags); err != nil {
klog.Fatalf("Error in reading and validating flags: %v", err)
}

// fail early if we don't have enough privileges
Expand Down
2 changes: 1 addition & 1 deletion pkg/cmd/showConfig.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ func NewShowConfigCommand(ioStreams genericclioptions.IOStreams) *cobra.Command
cfg.NodeName = ""
case "effective":
// Load the current configuration
if err := cfg.ReadAndValidate("", cmd.Flags()); err != nil {
if err := cfg.ReadAndValidate(config.GetConfigFile(), cmd.Flags()); err != nil {
cmdutil.CheckErr(err)
}
default:
Expand Down
11 changes: 4 additions & 7 deletions pkg/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,8 +183,6 @@ func StringInList(s string, list []string) bool {
return false
}

// Note: add a configFile parameter here because of unit test requiring custom
// local directory
func (c *MicroshiftConfig) ReadFromConfigFile(configFile string) error {
contents, err := os.ReadFile(configFile)
if err != nil {
Expand Down Expand Up @@ -243,11 +241,10 @@ func (c *MicroshiftConfig) ReadFromCmdLine(flags *pflag.FlagSet) error {
// Note: add a configFile parameter here because of unit test requiring custom
// local directory
func (c *MicroshiftConfig) ReadAndValidate(configFile string, flags *pflag.FlagSet) error {
if configFile == "" {
configFile = findConfigFile()
}
if err := c.ReadFromConfigFile(configFile); err != nil {
return err
if configFile != "" {
Copy link
Contributor

@dhellmann dhellmann Nov 1, 2022

Choose a reason for hiding this comment

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

Should an empty filename be an error now?

Copy link
Contributor Author

@benluddy benluddy Nov 1, 2022

Choose a reason for hiding this comment

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

It wasn't in the past -- that seems to have been an inadvertent change of the original PR.

Copy link
Contributor

@dhellmann dhellmann Nov 1, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@benluddy benluddy Nov 1, 2022

Choose a reason for hiding this comment

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

/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.

Copy link
Contributor

@dhellmann dhellmann Nov 1, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@copejon copejon Nov 1, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@benluddy benluddy Nov 1, 2022

Choose a reason for hiding this comment

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

/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.

if err := c.ReadFromConfigFile(configFile); err != nil {
return err
}
}
if err := c.ReadFromEnv(); err != nil {
Copy link
Contributor

@dhellmann dhellmann Nov 1, 2022

Choose a reason for hiding this comment

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

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.

return err
Expand Down