-
Notifications
You must be signed in to change notification settings - Fork 55
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 package for loading application config #89
Conversation
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.
I'm a big fan of this draft. I think this will be a really nice qol upgrade, and should help out a ton to remove these "default" style configs from repos.
Do you think it would be nice/reasonable to create a "global" default fallback, after organization specific checks?
My current thought for this is that it's something implemented in each application. If loading config returns an undefined value, the application can choose to either substitute a default config or ignore the repo. That gives more flexibility in how the default is defined and means you don't have to encode it as a |
Given some of the aims we have at the moment with internal github apps and config centralization I'm massively in favour of this as drafted. I think the current method and approach makes sense to me. I don't see the use-case for loading configuration from a private repo in another org, feels very strange to me and liable to leak whatever they're trying to keep secret anyway, but I'm not familiar with the original request
|
Implemented the core logic and added some basic tests, so I think this is ready for review. |
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.
Largely in support of this PR! I have a couple of small qs about the config, but I think it makes a lot of sense and will be really valuable.
func NewLoader(paths []string, opts ...Option) *Loader { | ||
defaultPaths := make([]string, len(paths)) | ||
for i, p := range paths { | ||
defaultPaths[i] = strings.TrimPrefix(p, ".github/") |
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.
Why does this path get trimmed out?
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.
Mostly just default behavior I decided on. The idea is that if you are using a .github
repository, you should be able to set configuration for the .github
repository that is different from the configuration it contains for all the other organization repositories. If the path was the same, the configuration you used for all repos would have to be the same as the configuration used for the .github
repository itself.
This is all customizable with the WithOwnerDefault
option.
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.
I should also add that stripping this path matches the idea of the .github
repo being like the shared contents of a .github
folder in a single repo.
// Referencing a remote file that does not exist is an error because | ||
// this condition is annoying to debug otherwise. From the perspective | ||
// of a repository, it appears that the application has a configuration | ||
// file and it is easy to miss that e.g. the ref is wrong. |
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.
🥇
ld := Loader{ | ||
paths: paths, | ||
parser: YAMLRemoteRefParser, | ||
defaultRepo: ".github", |
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.
Should this be a configurable server option?
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.
This is customizable with the WithOwnerDefault
option.
Merging this for now, will try to put a prototype conversion for policy-bot or bulldozer later this week. |
The
appconfig
package defines a standard (but customizable) flow for loading repository-level configuration:.github
) repositoryThe package doesn't require the configuration to have any format (it returns raw bytes), but by default assumes that remote references are encoded as YAML (this is customizable.)
This will hopefully make it easier to implement a consistent config loading mechanism in our apps.
Right now, this contains the public interface and what I think are all the exported types. The logic for actually loading config is unimplemented, so I'm most interested in comments on the API. Some starting questions/thoughts:
Loader
interface because there's only one implementation. But since there's only one method, it's easy enough for clients to define the necessary interface if they want to provide mocking or different implementations.githubapp
. Sinceoauth2
is already at the top-level, I stuck with that convention instead of nesting it ingithubapp
.appconfig
felt like a good name becauseconfig
was too generic, but I'm open to other suggestions if anyone thinksappconfig
will get confused with the server configuration.