Skip to content

Conversation

@marianogappa
Copy link
Contributor

@marianogappa marianogappa commented Oct 14, 2024

Prerequisite to fully implement: cloudquery/cloudquery#19286

In order to let CLI users when the plugins they are using are not up to date, we need to now what the latest version is.

This PR only adds this functionality within plugin-pb-go within the managedplugin module, but it isn't yet used, nor compared to the user's plugin version.

Sample check on CLI to see if it works:

$ go run . test-connection /Users/mariano.gappa/Code/test-aws-sync/all.yml

Loading spec(s) from /Users/mariano.gappa/Code/test-aws-sync/all.yml
You chose version v27.23.0. Latest version is v27.23.1.

Also note that this implementation errors in a lot of cases, but it is not the intention to actually error to the client; since the purpose is to show a warning, we'll probably elide any warnings (it's debatable) if there's no sufficient information to show a warning.

@marianogappa marianogappa requested review from a team and erezrokah and removed request for a team October 14, 2024 07:03
@github-actions github-actions bot added the feat label Oct 14, 2024
@github-actions github-actions bot added feat and removed feat labels Oct 14, 2024
return "", fmt.Errorf("team name is required to find the latest plugin version")
}

resp, err := c.ListPluginVersionsWithResponse(ctx, ops.PluginTeam, cloudquery_api.PluginKind(ops.PluginKind), ops.PluginName, &cloudquery_api.ListPluginVersionsParams{})
Copy link
Member

Choose a reason for hiding this comment

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

GetPlugin has a LatestVersion field which leads to less data being transferred and leaves the page management/ordering stuff to the backend.

Copy link
Member

Choose a reason for hiding this comment

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

resp, err := c.GetPluginWithResponse(ctx, ops.PluginTeam, cloudquery_api.PluginKind(ops.PluginKind), ops.PluginName)
Use resp.JSON200.LatestVersion

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 reworked the code based on:

  • How it's gonna be used. It didn't work well as a standalone function. I put it in the Client that sync gets.
  • I replaced the API call to use GetPluginWithResponse as per feedback.

Now, a sample usage in CLI looks like this:

latestVersion, err := destPluginClient.FindLatestPluginVersion(ctx, managedplugin.PluginDestination)

And a sample check to see if it works:

$ go run . test-connection /Users/mariano.gappa/Code/test-aws-sync/all.yml

Loading spec(s) from /Users/mariano.gappa/Code/test-aws-sync/all.yml
You chose version v27.23.0. Latest version is v27.23.1.

Copy link
Member

@disq disq left a comment

Choose a reason for hiding this comment

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

Use GetPluginWithResponse as in the comment :)

@marianogappa marianogappa requested a review from disq October 14, 2024 08:34
@github-actions github-actions bot added feat and removed feat labels Oct 14, 2024
return "", fmt.Errorf("failed to get hub client: %w", err)
}

if ops.TeamName == "" {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe check c.TeamName (as early as possible, around where c.config.Registry is checked) directly so it's easier to read.

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 checked it at the top now.

}

if resp.JSON200.LatestVersion == nil {
return "", fmt.Errorf("no latest plugin version found")
Copy link
Member

Choose a reason for hiding this comment

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

Could this be made a silencable error? Maybe return "", nil? You could also export an ErrNoLatestVersion so that it's more explicit, but it's a very rare scenario (using unpublished plugins...) I don't think a separate error is needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the purpose of this call is to emit a warning (not an error), wouldn't it be better to leave this as is (there wasn't a plugin version, so it makes sense to error), and in the calling code never error, but only emit the warning when we know the plugin is outdated?

Copy link
Member

Choose a reason for hiding this comment

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

Not having a latest plugin version can be normal/expected sometimes, but the code is "upgrading" it to an error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

@disq disq left a comment

Choose a reason for hiding this comment

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

LGTM with nits :)

@marianogappa marianogappa added the automerge Add to automerge PRs once requirements are met label Oct 14, 2024
@kodiakhq kodiakhq bot merged commit eac217c into main Oct 14, 2024
@kodiakhq kodiakhq bot deleted the mariano/find-latest-plugin-version branch October 14, 2024 12:12
@erezrokah
Copy link
Member

Commenting from an internal discussion, the code in this PR won't work for unauthenticated users. Let's say someone is using an offline license, or has the plugins cached. They don't need to be authenticated to check for the latest version

@erezrokah
Copy link
Member

erezrokah commented Oct 14, 2024

Commenting from an internal discussion, the code in this PR won't work for unauthenticated users. Let's say someone is using an offline license, or has the plugins cached. They don't need to be authenticated to check for the latest version

I was wrong, it doesn't complain if the token is not provided

if ops.AuthToken != "" {

kodiakhq bot pushed a commit that referenced this pull request Oct 14, 2024
🤖 I have created a release *beep* *boop*
---


## [1.23.0](v1.22.4...v1.23.0) (2024-10-14)


### Features

* Implement API to get latest plugin version. ([#415](#415)) ([eac217c](eac217c))


### Bug Fixes

* **deps:** Update module github.com/cloudquery/cloudquery-api-go to v1.13.1 ([#416](#416)) ([e02834b](e02834b))
* **deps:** Update module github.com/schollz/progressbar/v3 to v3.16.1 ([#410](#410)) ([b96b6da](b96b6da))
* **deps:** Update module google.golang.org/grpc to v1.67.1 ([#412](#412)) ([b52b41c](b52b41c))
* **deps:** Update module google.golang.org/protobuf to v1.35.1 ([#414](#414)) ([700ffa6](700ffa6))
* Generate Go Code from `plugin-pb` ([#413](#413)) ([9bcc208](9bcc208))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automerge Add to automerge PRs once requirements are met feat

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants