-
-
Notifications
You must be signed in to change notification settings - Fork 202
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
feat(config): add api config for branching override #2761
base: develop
Are you sure you want to change the base?
Conversation
@@ -2,7 +2,7 @@ | |||
// Use of this source code is governed by a BSD-style | |||
// license that can be found in the LICENSE file. | |||
|
|||
package link | |||
package diff |
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.
Couldn't put this under utils
because import in config
would cause cyclic dependencies.
RemoteApi struct { | ||
Enabled bool `toml:"enabled"` | ||
Schemas []string `toml:"schemas"` | ||
ExtraSearchPath []string `toml:"extra_search_path"` | ||
MaxRows uint `toml:"max_rows"` | ||
} |
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.
question:
This is what I ended up with, this is public facing so it can be imported inside branching
and we can use it's methods. And basically contain the fields that will be actually shared between remote and local config.
api struct { | ||
RemoteApi | ||
Image string `toml:"-"` | ||
KongImage string `toml:"-"` | ||
Port uint16 `toml:"port"` | ||
Tls tlsKong `toml:"tls"` | ||
// TODO: replace [auth|studio].api_url | ||
ExternalUrl string `toml:"external_url"` | ||
} |
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.
The cli specifics config remains in a private dedicated structure.
if err := base.Validate(fsys); err != nil { | ||
fmt.Fprintf(os.Stderr, "Error with remote config %s\n", name) |
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.
remark
While testing, when I set up a remote with project_id = ""
I had the error raising about the "whole config". Not mentioning the actual issue was inside one of the remote definition. This should help provide better guidance in such cases.
Pull Request Test Coverage Report for Build 11305072730Details
💛 - Coveralls |
What kind of change does this PR introduce?
Feature add tooling around api config to be used inside branching for config override.
What is the current behavior?
Please link any relevant issues here.
What is the new behavior?
Feel free to include screenshots if it includes visual changes.
Additional context
Add any other context or screenshots.