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

Plugins: Add managed instance installation resources #76767

Merged
merged 9 commits into from
Oct 24, 2023

Conversation

oshirohugo
Copy link
Contributor

@oshirohugo oshirohugo commented Oct 18, 2023

What is this feature?

This PR adds the necessary changes to create a new plugin installer for plugins running in managed instance:

  • add new properties to config
  • exposes methods to be shared with the cloud plugin installer
  • Move plugin installer bindings to OSS wire

Why do we need this feature?

This work is necessary to create a manage plugin installer, which is an alternative implementation of plugins.Installer interface and will allow plugins to be installed in a managed instance without being redirected to gcom.

Who is this feature for?

Grafana developers

Which issue(s) does this PR fix?:

This is part of https://github.com/grafana/grafana-plugins-platform-team/issues/28

Special notes for your reviewer:

The corresponding enterprise PR: https://github.com/grafana/grafana-enterprise/pull/5915

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

@oshirohugo oshirohugo requested review from torkelo and a team as code owners October 18, 2023 15:20
@oshirohugo oshirohugo requested review from wbrowne, marefr, xnyo, papagian, zserge and nikimanoledaki and removed request for a team October 18, 2023 15:20
@grafana-delivery-bot grafana-delivery-bot bot added this to the 10.3.x milestone Oct 18, 2023
Comment on lines 50 to 52
StackID string
InstallToken string
ExternalManageEnabled bool
Copy link
Member

Choose a reason for hiding this comment

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

I think it may be possible to get away without adding these to this config - we try to reserve this struct for core internal configuration where possible - so I think we can just inject *setting.Cfg where we want this info instead. WDYT?

Comment on lines +1505 to +1509
# Auth token for plugin installations and removal in managed instances
install_token =
Copy link
Contributor

Choose a reason for hiding this comment

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

can you verify if is it okay to put a token in configuration? Maybe we have a better place for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@andresmgot andresmgot left a comment

Choose a reason for hiding this comment

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

Code LGTM!

Copy link
Member

@wbrowne wbrowne left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -204,6 +204,7 @@ type FakePluginRepo struct {
GetPluginArchiveFunc func(_ context.Context, pluginID, version string, _ repo.CompatOpts) (*repo.PluginArchive, error)
GetPluginArchiveByURLFunc func(_ context.Context, archiveURL string, _ repo.CompatOpts) (*repo.PluginArchive, error)
GetPluginArchiveInfoFunc func(_ context.Context, pluginID, version string, _ repo.CompatOpts) (*repo.PluginArchiveInfo, error)
PluginVersionFunc func(pluginID, version string, compatOpts repo.CompatOpts) (repo.VersionData, error)
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Would be good to have context.Context here but you can save that a follow up if you prefer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer to make it in a follow up request. PluginVersionFunc is a mock of PluginVersion and PluginVersion is just a the old pluginVersion exported to reuse some code.

"time"
)

func MakeHttpClient(skipTLSVerify bool, timeout time.Duration) http.Client {
Copy link
Member

Choose a reason for hiding this comment

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

I think you could probably have this one in the repo package. WDYT?

@oshirohugo oshirohugo force-pushed the add-managed-instance-installation branch from ef9af1c to 75293f6 Compare October 24, 2023 08:02
@oshirohugo
Copy link
Contributor Author

@wbrowne all fixed, could you check it again?

@oshirohugo oshirohugo force-pushed the add-managed-instance-installation branch from 75293f6 to 2d8af72 Compare October 24, 2023 12:39
@oshirohugo oshirohugo merged commit dfc1875 into main Oct 24, 2023
14 checks passed
@oshirohugo oshirohugo deleted the add-managed-instance-installation branch October 24, 2023 14:21
@oshirohugo oshirohugo self-assigned this Nov 8, 2023
@aangelisc aangelisc modified the milestones: 10.3.x, 10.2.3 Dec 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants