Skip to content
This repository has been archived by the owner on Mar 24, 2022. It is now read-only.

config file support #9

Open
jtarchie opened this issue Sep 6, 2018 · 5 comments
Open

config file support #9

jtarchie opened this issue Sep 6, 2018 · 5 comments

Comments

@jtarchie
Copy link
Contributor

jtarchie commented Sep 6, 2018

The command line arguments being used in om sometimes represent large JSON payloads. For example, the configure-product --product-properties. This command has added support for a --config YAML file, so those command line arguments can be passed in -- over bash encoding a large JSON string.

We were investigating adding this behaviour to other commands on om, too. Since, --config file is mapping a file to command line arguments, we think jhanda might be a great place to centralize this logic.

Our team's proposal, is to have a special ConfigFile type for that if it exists on the struct will automatically load from the file.

Better to show than tell. If we were to write a test.

	var options struct {
		First string     `long:"first" env:"FIRST"`
		File  ConfigFile `long:"config"`
	}

	_, err := jhanda.Parse(&options, []string{"--first", "hello"})
	Expect(err).ToNot(HaveOccurred())
	Expect(options.First).To(Equal("hello"))

	// assume this file exists
	// config.yaml
	// ---
	// first: world
	_, err := jhanda.Parse(&options, []string{"--config", "config.yml"})
	Expect(err).ToNot(HaveOccurred())
	Expect(options.First).To(Equal("hello"))

	_, err := jhanda.Parse(&options, []string{"--first", "testing", "--config", "config.yml"})
	Expect(err).ToNot(HaveOccurred())
	Expect(options.First).To(Equal("testing"))

	// assume environment variable
	// FIRST="I am env var"
	_, err := jhanda.Parse(&options, []string{"--config", "config.yml"})
	Expect(err).ToNot(HaveOccurred())
	Expect(options.First).To(Equal("I am env var"))
	
	// matching existing functionality: flag > env var 
	// assume environment variable
	// FIRST="I am env var"
	_, err := jhanda.Parse(&options, []string{"--first", "testing", "--config", "config.yml"})
	Expect(err).ToNot(HaveOccurred())
	Expect(options.First).To(Equal("testing"))
	
	// so importance to set the param would be flag > env var > config file
@cf-gitbot
Copy link

We have created an issue in Pivotal Tracker to manage this. Unfortunately, the Pivotal Tracker project is private so you may be unable to view the contents of the story.

The labels on this github issue will be updated when the story is started.

@ljfranklin
Copy link
Contributor

This seems interesting as a more holistic solution to flag parsing. The only issue that jumps out immediately is --config is overloaded as commands like configure-product already have a --config flag. Maybe there's a better name? Changing the name of the existing --config flags as a breaking change is also an option if we feel like --config is the best name for the flag that reads flags.

@jtarchie
Copy link
Contributor Author

jtarchie commented Sep 7, 2018

@ljfranklin, the implication above is that it would be a backwards compatible change with --config. Just moving the logic that those commands have for loading directly from a single file from om to jhanda.

Of course, when I say this, I do realize this would affect the interpolation that om does on those files too. 🤔

@mcwumbly
Copy link

mcwumbly commented Sep 7, 2018

I don't have a good feel for whether this belongs in here, or if it'll take some time to gain alignment on the design.

But if it's feeling like it's going to take time to hash out the design or decide, one option in the short term may be to first wrap jhanda inom to add this behavior for now. That could solve the om problem of centralizing this responsibility and then offer a reference implementation to consider pushing down into jhanda.

@ljfranklin
Copy link
Contributor

the implication above is that it would be a backwards compatible change with --config

Right, I forgot that the config file that configure-product takes in has top-level keys that match the flag names. We do have other commands like create-vm-extension where this doesn't hold:


ॐ  create-vm-extension
This creates/updates a VM extension

Usage: om [options] configure-product [<args>]
  --client-id, -c, OM_CLIENT_ID          string  Client ID for the Ops Manager VM (not required for unauthenticated commands)
  --client-secret, -s, OM_CLIENT_SECRET  string  Client Secret for the Ops Manager VM (not required for unauthenticated commands)
  --connect-timeout, -o                  int     timeout in seconds to make TCP connections (default: 5)
  --format, -f                           string  Format to print as (options: table,json) (default: table)
  --help, -h                             bool    prints this usage information (default: false)
  --password, -p, OM_PASSWORD            string  admin password for the Ops Manager VM (not required for unauthenticated commands)
  --request-timeout, -r                  int     timeout in seconds for HTTP requests to Ops Manager (default: 1800)
  --skip-ssl-validation, -k              bool    skip ssl certificate validation during http requests (default: false)
  --target, -t, OM_TARGET                string  location of the Ops Manager VM
  --trace, -tr                           bool    prints HTTP requests and response payloads
  --username, -u, OM_USERNAME            string  admin username for the Ops Manager VM (not required for unauthenticated commands)
  --version, -v                          bool    prints the om release version (default: false)

Command Arguments:
  --cloud-properties, -cp  string (required)  cloud properties in JSON format
  --config, -c             string             path to yml file containing all config fields (see docs/create-vm-extension/README.md for format)
  --name, -n               string (required)  VM extension name
  --ops-file, -o           string (variadic)  YAML operations file
  --vars-file, -l          string (variadic)  Load variables from a YAML file

Configuring via file

Example YAML:

vm-extension-config:
  name: some_vm_extension
  cloud_properties:
    source_dest_check: false

But changing the top-level key in the create-vm-extension config file to match the flag name or vice versa seems doable.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants