Extend the GitHub App configuration with an autoRegistration object #3449
Conversation
|
@blkt I accepted your suggestion, can you re-review? |
In order to actually set the auto-registration of entities, we need to
be able to configure those options.
This patch adds two structures: AutoRegistration which contains a list
of entities to auto-register and ProviderConfig which contains the
AutoRegistration.
The idea behind this is that any provider would be able to use the
ProviderConfig attributes regardless of the provider type. So as an
example, both would be valid:
```
{ "auto_registration": { "enabled": ["repository"] }, "github": { "endpoint": "https://api.github.com" }}
```
and:
```
{ "auto_registration": { "enabled": ["repository"] }, "github-app": { "endpoint": "https://api.github.com" }}
```
and all would require just embedding the ProviderConfig structure into
the provider-specific parser.
Fixes: mindersec#3266
Amends the ParseV1AppConfig to also include parsing the minderv1.GitHubAppProviderConfig structure. The parsed-out object is not used yet, but it's parsed and validated and the new code is tested. Related: mindersec#3266
jhrozek
force-pushed
the
auto_register_config
branch
from
35ac227
to
b1ec441
Compare
There was a problem hiding this comment.
One minor note, sorry I didn't get to this earlier.
I'm assuming that some portion of the parameters like GitHubProviderConfig, GitHubAppProviderConfig, DockerHubProviderConfig, etc are not fully hooked up at the moment, but we should think about what the documentation looks like once they are hooked up -- I'm guessing that we'd want some sort of page which documents the supported fields for e.g. the GitHubApp provider.
| // AutoRegistration is the configuration for auto-registering entities. | ||
| // When nothing is set, it means that auto-registration is disabled. There is no difference between disabled | ||
| // and undefined so for the "let's not auto-register anything" case we'd just let the repeated string empty | ||
| message AutoRegistration { | ||
| // enabled is the list of entities that are enabled for auto-registration. | ||
| repeated string enabled = 1; | ||
| } | ||
|
|
||
| // ProviderConfig contains the generic configuration for a provider. | ||
| message ProviderConfig { | ||
| // auto_registration is the configuration for auto-registering entities. | ||
| AutoRegistration auto_registration = 1; | ||
| } |
There was a problem hiding this comment.
I feel like we need some sort of documentation about how these messages line up (or don't) with other fields in the API. I'm assuming these are serialized as Struct / JSON in the config field of Provider?
There was a problem hiding this comment.
Yes, this is the next part I'm working on at the moment, I hope to get a PR out soon.
Thanks for going back and taking a look! I filed #3458 to ensure we don't forget about documentation. Hopefully how the configuration is updated will be more clear once I send the next PR. |
Summary
Adds the protobuf struct for generic provider options
In order to actually set the auto-registration of entities, we need to
be able to configure those options.
This patch adds two structures: AutoRegistration which contains a list
of entities to auto-register and ProviderConfig which contains the
AutoRegistration.
The idea behind this is that any provider would be able to use the
ProviderConfig attributes regardless of the provider type. So as an
example, both would be valid:
and:
and all would require just embedding the ProviderConfig structure into
the provider-specific parser.
Fixes: #3266
Parse out ProviderConfig as well when reading GH App configuration
Amends the ParseV1AppConfig to also include parsing the
minderv1.GitHubAppProviderConfig structure.
The parsed-out object is not used yet, but it's parsed and validated and the
new code is tested.
Related: #3266
Change Type
Mark the type of change your PR introduces:
Testing
make test for now
Review Checklist: