-
Notifications
You must be signed in to change notification settings - Fork 187
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
add OCIS_URL env var #1148
add OCIS_URL env var #1148
Changes from 1 commit
7dbc1af
af9900c
ff42549
03e1f95
26c82b5
f48b3f4
63ae28f
b2e827f
36d86aa
652448f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -167,7 +167,7 @@ func ServerWithConfig(cfg *config.Config) []cli.Flag { | |||||||||||||||||||||||||||||||||||||||||
&cli.StringFlag{ | ||||||||||||||||||||||||||||||||||||||||||
Name: "iss", | ||||||||||||||||||||||||||||||||||||||||||
Usage: "OIDC issuer URL", | ||||||||||||||||||||||||||||||||||||||||||
EnvVars: []string{"KONNECTD_ISS"}, | ||||||||||||||||||||||||||||||||||||||||||
EnvVars: []string{"KONNECTD_ISS", "OCIS_URL"}, // KONNECTD_ISS takes precedence over OCIS_URL | ||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While digging through the code I noticed, that this enables easy configuration for environment variable only. This will not picked up if you run For that to work something like for ocis/ocis/pkg/command/accounts.go Lines 41 to 60 in d089586
I'm not sure which is the way to go because it also has downsides. In this example There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes. the downside is known but accepted because the ocis binary is a poor mans orchestrator. I would argue that in eg docker compose deployments we should run the services in their own docker containers, which is why the multi repo setup was building a docker container for every service ... dunno if we still do that ... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we'll add the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. scratch that ... I thought we could add a |
||||||||||||||||||||||||||||||||||||||||||
Value: "https://localhost:9200", | ||||||||||||||||||||||||||||||||||||||||||
Destination: &cfg.Konnectd.Iss, | ||||||||||||||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docs pick up only the first environment variable. What should we do about this? Document it differently or do a PR to https://github.com/owncloud/flaex?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR to flaex is the correct fix IMO. Cc @IljaN
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good 👍