Skip to content
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.

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

[full-ci] Revamp Config Parsing #2708

Merged
merged 171 commits into from Nov 23, 2021
Merged

[full-ci] Revamp Config Parsing #2708

merged 171 commits into from Nov 23, 2021

Conversation

refs
Copy link
Member

@refs refs commented Oct 29, 2021

TODO

  • add changelog
  • remove all flagsets
  • move all flagset default values into structs
  • use gookit/config to aid with config and env vars parsing
  • remove any dependencies on viper
  • remove the docs pipeline on CI
  • provide with a config docs section
  • action: how do we want to generate docs for the new structs? We can reuse the old Description field in the flags and have a go:generate do the work see Generate config docs #2806
  • ...

This is an implementation of the proposed configuration proposal on #2704

  • it adds a new dependency that does roughly what I started to do on a side project. Not reinventing the wheel. MIT license.
  • remove cli flags on subcommands, not on ocis root command
  • makes easy to do a config dump
  • support for environment variables
  • support for merging config values
  • support for defaults
  • and most important thread safe, as I can create as many config.Config structs as I wish.

This is also in preparation for the upcoming "restart a service if a config changes" story.

ENV Variables and Templates

We have 2 options. Let the ENV parsing to the config file, which looks like:

proxy.yaml:

log:
  level: info
  color: true
  pretty: true
http:
  addr: "${PROXY_HTTP_ADDR|localhost:2222}"

There clearly PROXY_HTTP_ADDR will take precedence over the default specified in the config file localhost:2222.

The other alternative is to parse a set of predefined ENV variables, get the config environment and overwrite the config values, trimming down the config a bit.

I personally prefer the first approach, since it allows more flexibility for the end user as they are in control of the environment, and we only need to provide with a default in-memory config file that reads from the default environment values, in the case there is no extension.[*] config file on the user's machine, this way we don't break config parsing in the case the user wants to use only environment variables.

@update-docs
Copy link

update-docs bot commented Oct 29, 2021

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@refs refs changed the title remove flagset on proxy, use default config values Revamp Config Parsing Oct 29, 2021
@refs refs mentioned this pull request Oct 29, 2021
@refs refs self-assigned this Oct 29, 2021
proxy/pkg/command/root.go Outdated Show resolved Hide resolved
@kobergj
Copy link
Collaborator

kobergj commented Oct 29, 2021

@refs I'm not sure about approach No1. Would that work with any configuration type? (yml? json? xml?)

@refs
Copy link
Member Author

refs commented Oct 29, 2021

@refs I'm not sure about approach No1. Would that work with any configuration type? (yml? json? xml?)

yes, supports many encoders: https://github.com/gookit/config/blob/8a7c1f28b5b0e9abecffc94d0d865e17a531eb0d/config.go#L87

@ownclouders
Copy link
Contributor

ownclouders commented Nov 2, 2021

💥 Acceptance test Core-API-Tests-ocis-storage-10 failed. Further test are cancelled...

@owncloud owncloud deleted a comment from ownclouders Nov 2, 2021
@owncloud owncloud deleted a comment from ownclouders Nov 2, 2021
@owncloud owncloud deleted a comment from ownclouders Nov 2, 2021
@owncloud owncloud deleted a comment from ownclouders Nov 2, 2021
@owncloud owncloud deleted a comment from ownclouders Nov 2, 2021
@owncloud owncloud deleted a comment from ownclouders Nov 2, 2021
@owncloud owncloud deleted a comment from ownclouders Nov 2, 2021
@owncloud owncloud deleted a comment from ownclouders Nov 2, 2021
@owncloud owncloud deleted a comment from ownclouders Nov 2, 2021
@owncloud owncloud deleted a comment from ownclouders Nov 2, 2021
@owncloud owncloud deleted a comment from ownclouders Nov 2, 2021
@owncloud owncloud deleted a comment from ownclouders Nov 2, 2021
@owncloud owncloud deleted a comment from ownclouders Nov 2, 2021
@owncloud owncloud deleted a comment from ownclouders Nov 2, 2021
@owncloud owncloud deleted a comment from ownclouders Nov 2, 2021
.drone.star Outdated Show resolved Hide resolved
docs/ocis/config.md Outdated Show resolved Hide resolved
@refs refs requested a review from wkloucek November 20, 2021 10:21
@refs refs mentioned this pull request Nov 23, 2021
@sonarcloud
Copy link

sonarcloud bot commented Nov 23, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 13 Code Smells

6.8% 6.8% Coverage
24.9% 24.9% Duplication

Copy link
Contributor

@wkloucek wkloucek left a comment

Choose a reason for hiding this comment

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

Let's see that out in the wild 🚀

Copy link
Contributor

@micbar micbar left a comment

Choose a reason for hiding this comment

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

Let it fly ✈️

@refs refs merged commit 1befac2 into master Nov 23, 2021
@refs refs deleted the try-gookikt-config branch November 23, 2021 12:18
ownclouders pushed a commit that referenced this pull request Nov 23, 2021
Merge: 3bc413b 43af041
Author: Alex Unger <6905948+refs@users.noreply.github.com>
Date:   Tue Nov 23 13:18:39 2021 +0100

    Merge pull request #2708 from owncloud/try-gookikt-config
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.

None yet

5 participants