Skip to content

feat: Offline licensing support#1436

Merged
disq merged 7 commits into
cloudquery:mainfrom
disq:feat/offline-licensing
Dec 28, 2023
Merged

feat: Offline licensing support#1436
disq merged 7 commits into
cloudquery:mainfrom
disq:feat/offline-licensing

Conversation

@disq
Copy link
Copy Markdown
Member

@disq disq commented Dec 28, 2023

BEGIN_COMMIT_OVERRIDE
feat: Offline licensing support

Includes breaking changes to the premium.NewUsageClient interface. Save value of opts.PluginMeta from Configure() and pass it to premium.NewUsageClient as the first argument.
END_COMMIT_OVERRIDE

@github-actions github-actions Bot added the feat label Dec 28, 2023
@github-actions github-actions Bot added feat and removed feat labels Dec 28, 2023
@disq disq marked this pull request as ready for review December 28, 2023 15:58
@github-actions github-actions Bot added feat and removed feat labels Dec 28, 2023
@disq disq changed the title feat: Offline licensing feat: Offline licensing support Dec 28, 2023
@github-actions github-actions Bot added feat and removed feat labels Dec 28, 2023
Comment thread premium/offline.go Outdated
Comment thread premium/usage.go Outdated
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Dec 28, 2023

⏱️ Benchmark results

Comparing with 1fdf892

  • Glob-8 ns/op: 91.1 (no change)

Comment thread premium/usage.go
}

func NewUsageClient(pluginTeam cqapi.PluginTeam, pluginKind cqapi.PluginKind, pluginName cqapi.PluginName, ops ...UsageClientOptions) (*BatchUpdater, error) {
func NewUsageClient(meta plugin.Meta, ops ...UsageClientOptions) (UsageClient, error) {
Copy link
Copy Markdown
Contributor

@hermanschaaf hermanschaaf Dec 28, 2023

Choose a reason for hiding this comment

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

I agree this signature change is an improvement, but it's technically a breaking change, and we'll have to update all plugins using the premium package. Maybe we should do a v5 release for this?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

NewUsageClient is used only in CQ-authored, effectively private plugins.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That might be true but we have community plugins that are premium as well. And if they're not using it, they should be

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Specifically the Azure AD, Box and IPInfo plugins are premium community plugins written in Go, and might also need updates. If we want to reach out to those authors I suppose that's also fine

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I don't think that warrants an upgrade of ~7000 lines in the public cq/cq repo.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can't we just avoid this by keeping the signature the same? It looks like we can adopt UsageClientOptions for the additional option we need

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh I guess the *BatchUpdater return might still be an issue. Well let's at least make a clear note in the changelog that this is a breaking change to the premium package interface, and explain what plugin authors need to do to handle it.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

There are ways to avoid this by shifting the burden to the plugin side, which is not favourable.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah we don't need to go that far; let's just add an explanation to the changelog

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I've updated the PR desc. Adding a BREAKING-CHANGE there might bump the version, but further edits or suggestions are welcome.

@github-actions github-actions Bot added feat and removed feat labels Dec 28, 2023
@disq disq merged commit 1fdf892 into cloudquery:main Dec 28, 2023
@disq disq deleted the feat/offline-licensing branch December 28, 2023 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants