Skip to content

feat(config): add config overriding capability with pflags#30

Closed
deryrahman wants to merge 2 commits into
mainfrom
feat-config-pflags-capability
Closed

feat(config): add config overriding capability with pflags#30
deryrahman wants to merge 2 commits into
mainfrom
feat-config-pflags-capability

Conversation

@deryrahman
Copy link
Copy Markdown
Member

@deryrahman deryrahman commented Apr 6, 2022

Providing capability to replace config via flags

    flags := cmd.Flags()
    loader := config.NewLoader(
        config.WithName("config"),
        config.WithType("yaml"),
        config.WithPFlags(flags, "-"),
    )

@deryrahman deryrahman changed the title feat(config): add config override capability with pflags feat(config): add config overriding capability with pflags Apr 6, 2022
@ravisuhag ravisuhag requested a review from rohilsurana April 6, 2022 08:07
@deryrahman deryrahman force-pushed the feat-config-pflags-capability branch from 9aa880f to bfdb499 Compare April 6, 2022 08:21
@deryrahman deryrahman self-assigned this Apr 6, 2022
@rohilsurana
Copy link
Copy Markdown
Member

Since we don't expect SetPFlagsKeyDelimiter without the use of WithPFlags, does it makes sense to combine them into one option that would remove the need for having them called in order?


Need to update the readme to add usage and remove todo point.

@deryrahman
Copy link
Copy Markdown
Member Author

does it makes sense to combine them into one option that would remove the need for having them called in order?

@rohilsurana Yes it does. Would it be better if we use other func name? Or using this func name WithPFlags(flags, "-") is sufficient? Let me know if you have other suggestion

Need to update the readme to add usage and remove todo point.

Will do that

@rohilsurana
Copy link
Copy Markdown
Member

That name looks good.

Comment thread config/README.md
func main() {
var c Config
cmd := cobra.Command{}
cmd.Flags().Int("db-port", 5432, "set db port")
Copy link
Copy Markdown
Member

@rohilsurana rohilsurana Apr 8, 2022

Choose a reason for hiding this comment

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

Looking at this usage example, can we automate setting the flag definitions?

Since we support setting the default values in the struct, there can be a mismatch in the value set by flag vs that in the struct. Basically we won't have a single source of truth.

So we have 2 options here -

  • Support defining flags automatically with this config package (Not sure how complex this would be)
  • Remove support for default values from struct and only use default values from flags. (Easy, but this option has an issue that folks not using flags will need to figure out their own way to set defaults)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@rohilsurana @deryrahman Any final decision on this?

Copy link
Copy Markdown
Member Author

@deryrahman deryrahman Jul 21, 2022

Choose a reason for hiding this comment

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

not yet @ravisuhag , deferring the discussion as I haven't spiked the approach yet

I strongly suggest not to remove the support default values from struct. Will back to this once the spiking is done

@rohilsurana
Copy link
Copy Markdown
Member

#48 adds a way to override flags when using cmdx package as well.

@ravisuhag
Copy link
Copy Markdown
Member

@rohilsurana yes, closing this for now.

@ravisuhag ravisuhag closed this Sep 12, 2022
@ravisuhag ravisuhag deleted the feat-config-pflags-capability branch October 19, 2022 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants