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

Added minder provider update CLI command. #3676

Merged
merged 2 commits into from
Jun 20, 2024
Merged

Added minder provider update CLI command. #3676

merged 2 commits into from
Jun 20, 2024

Conversation

blkt
Copy link
Contributor

@blkt blkt commented Jun 19, 2024

Summary

Update command allows changing configuration for provider on a per-field basis. It operates on a read-modify-write fashion, retrieving current configuration from the backend for modification.

Modification is done by reflection by walking over the struct's JSON tags. Implementation is recursive, but the maximum depth is determined by the deepest field in the configuration struct.

Argument parsing is trivial and assumes that arguments are either of the form field1.field2.field3 (for --unset-attribute) or field1.field2.field3=value (for --set-attribute). The right parser for value is determined once the correct struct field is found.

It is not currently possible to modify "github-app" or "github" fields.

Fixes #3509

Change Type

Mark the type of change your PR introduces:

  • Bug fix (resolves an issue without affecting existing features)
  • Feature (adds new functionality without breaking changes)
  • Breaking change (may impact existing functionalities or require documentation updates)
  • Documentation (updates or additions to documentation)
  • Refactoring or test improvements (no bug fixes or new functionality)

Testing

  • unit tested
  • locally tested
  • tested against SAAS (staging)

Review Checklist:

  • Reviewed my own code for quality and clarity.
  • Added comments to complex or tricky code sections.
  • Updated any affected documentation.
  • Included tests that validate the fix or feature.
  • Checked that related changes are merged.

@blkt blkt requested a review from a team as a code owner June 19, 2024 16:58
@coveralls
Copy link

Coverage Status

coverage: 53.115% (-0.01%) from 53.125%
when pulling a3229ac on update_prov_cli
into cfeaed8 on main.

@coveralls
Copy link

Coverage Status

coverage: 53.12% (-0.005%) from 53.125%
when pulling a3229ac on update_prov_cli
into cfeaed8 on main.

jhrozek
jhrozek previously approved these changes Jun 19, 2024
Copy link
Contributor

@jhrozek jhrozek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great. I left some minor comments inline but I'm approving nonetheless so you can go ahead and merge.

cmd/cli/app/provider/provider_update.go Show resolved Hide resolved
cmd/cli/app/provider/provider_update.go Show resolved Hide resolved
cmd/cli/app/provider/provider_update.go Show resolved Hide resolved
cmd/cli/app/provider/provider_update.go Outdated Show resolved Hide resolved
//nolint:lll
GitHub *minderv1.GitHubProviderConfig `json:"github,omitempty" yaml:"github" mapstructure:"github" validate:"required"`
//nolint:lll
GitHubApp *minderv1.GitHubAppProviderConfig `json:"github_app,omitempty" yaml:"github_app" mapstructure:"github_app" validate:"required"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have any thoughts on how would we handle additional custom providers?

Copy link
Contributor Author

@blkt blkt Jun 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't looked too much into this, but I believe that custom providers should ship their configuration structure as some form of spec (e.g. JSON Spec) so that every client can perform validation and guarantee type safety when parsing/producing a config object. That would also make it possible to dynamically produce valid attribute names as help strings for example.

An additional benefit of this is that we could probably get rid of reflection altogether by walking over the spec rather than Go structs.

Moreover, configuration structure is a problem already, because it's untyped outside of Minder Server, which is why I had to duplicate that struct. I think for our main providers we should be already shipping a spec for the config, and allow (or mandate) custom providers to do the same in the future.

@coveralls
Copy link

Coverage Status

coverage: 52.78% (-0.3%) from 53.125%
when pulling 2df5588 on update_prov_cli
into cfeaed8 on main.

blkt added 2 commits June 20, 2024 10:29
Update command allows changing configuration for provider on a
per-field basis. It operates on a read-modify-write fashion,
retrieving current configuration from the backend for modification.

Modification is done by reflection by walking over the struct's JSON
tags. Implementation is recursive, but the maximum depth is determined
by the deepest field in the configuration struct.

Argument parsing is trivial and assumes that arguments are either of
the form `field1.field2.field3` (for `--unset-attribute`) or
field1.field2.field3=value` (for `--set-attribute`). The right parser
for `value` is determined once the correct struct field is found.

It is not currently possible to modify `"github-app"` or `"github"`
fields.

Fixes #3509
The command minder provider update operates on a per-provider basis,
which requires the name to be mandatory.
@coveralls
Copy link

Coverage Status

coverage: 52.775%. remained the same
when pulling 103448c on update_prov_cli
into 7a3bce6 on main.

@blkt blkt merged commit bef52d6 into main Jun 20, 2024
21 checks passed
@blkt blkt deleted the update_prov_cli branch June 20, 2024 10:49
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.

Add a CLI to update provider config
3 participants