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

Allow config parsing on supervise mode only with run subcommand #1931

Merged
merged 4 commits into from
Apr 20, 2021

Conversation

refs
Copy link
Member

@refs refs commented Apr 19, 2021

What?

What the descriptions says. Currenntly it is not possible to parse a single config file from an extension when running on supervised mode.

a) global ocis config file on /etc/ocis/ocis.yaml with the contents:

proxy:
  http:
    addr: "0.0.0.0:6200"
accounts:
  http:
    addr: "0.0.0.0:8888"
Storage:
  Reva:
    StorageMetadata:
      Port:
        HTTPAddr: "0.0.0.0:7777"

b) a proxy config file /etc/ocis/proxy.json with the contents:

{
        "http": {
                "addr": "0.0.0.0:5200"
        },
        "debug": {
                "addr": "0.0.0.0:5201"
        }
}

Running everything:

  1. run ocis server // What will happen? /etc/ocis/ocis.yaml will get parsed and all extensions will inherit their config from it
  2. run ocis kill proxy // What will happen? supervised proxy will die
  3. run ocis run proxy // What will happen? /etc/ocis/proxy.json gets parsed and the proxy subcommand will run with the changes (it will no longer run the values from /etc/ocis/ocis.yaml)
    this works for me with the changes on this branch: https://github.com/owncloud/ocis/tree/parse-config-when-run

it also supports this use case:

  1. run ocis server
  2. run ocis run proxy
    and you should end up with 2 proxy instances bound to different ports.

A known issue that needs to be considered is the following: What should we do when we have a global ocis.yaml config file configuring all services and individual config files i.e: proxy.yaml? What should be the outcome of the following sequence of commands:

  1. ocis server // with a global ocis.yaml in preconfigured locations (it configures a proxy)
  2. ocis kill proxy // with a single proxy.yaml in the expected preconfigured locations
  3. ocis run proxy

What could we expect from running (3) ? Should the proxy config in the global ocis.yaml take precedence over the single more granular proxy.yaml? Should both be considered and have their values merged?

The conundrum here is that we wouldn't only be talking here about source priorities on ENV var vs. CLI vs. config file but also between config files.

@update-docs
Copy link

update-docs bot commented Apr 19, 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
Copy link
Member Author

refs commented Apr 19, 2021

Semantics are still slightly confusing and will probably need to be worked upon to avoid misunderstandings within the code base.

@refs refs requested review from butonic and wkloucek April 19, 2021 13:33
@wkloucek
Copy link
Contributor

  1. run ocis server // What will happen? /etc/ocis/ocis.yaml will get parsed and all extensions will inherit their config from it
  2. run ocis kill proxy // What will happen? supervised proxy will die
  3. run ocis run proxy // What will happen? /etc/ocis/proxy.json gets parsed and the proxy subcommand will run with the changes (it will no longer run the values from /etc/ocis/ocis.yaml)

What if there is no /etc/ocis/proxy.json ? Will the proxy then be started with default settings? I find it confusing that ocis run does something different than ocis server.

@refs
Copy link
Member Author

refs commented Apr 19, 2021

What if there is no /etc/ocis/proxy.json ? Will the proxy then be started with default settings?

Yes, this is also an issue.

I find it confusing that ocis run does something different than ocis server.

Me too, and I don't like the inconsistency, that's why I added that last comment. It is confusing and it needs a better thought process, but we literally have our hands tied due to Viper not being thread safe setup for a bad design :/

@wkloucek
Copy link
Contributor

After #1929 I also run into this because I cannot configure the proxy with its configuration file. Even if I explicitly set it via environment variable PROXY_CONFIG_FILE=proxy.json.

@refs
Copy link
Member Author

refs commented Apr 20, 2021

A known issue that needs to be considered is the following: What should we do when we have a global ocis.yaml config file configuring all services and individual config files i.e: proxy.yaml? What should be the outcome of the following sequence of commands:

  1. ocis server // with a global ocis.yaml in preconfigured locations (it configures a proxy)
  2. ocis kill proxy // with a single proxy.yaml in the expected preconfigured locations
  3. ocis run proxy

What could we expect from running (3) ? Should the proxy config in the global ocis.yaml take precedence over the single more granular proxy.yaml? Should both be considered and have their values merged?

The conundrum here is that we wouldn't only be talking here about source priorities on ENV var vs. CLI vs. config file but also between config files.

@refs refs self-assigned this Apr 20, 2021
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

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

0.0% 0.0% Coverage
0.0% 0.0% 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.

LGTM as quick fix, create ADR as followup

@refs refs merged commit cff1325 into master Apr 20, 2021
@refs refs deleted the parse-config-when-run branch April 20, 2021 14:10
ownclouders pushed a commit that referenced this pull request Apr 20, 2021
Merge: c073329 60280a3
Author: Alex Unger <[email protected]>
Date:   Tue Apr 20 16:10:56 2021 +0200

    Merge pull request #1931 from owncloud/parse-config-when-run
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.

2 participants