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

install latest plugin when the modules are not released for the latest neo-cli #3229

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

Jim8y
Copy link
Contributor

@Jim8y Jim8y commented May 13, 2024

Description

Cli will not able to install plugin if the modules are not yet released. So this pr will help the cli to install the latest plugin that can find on github.

Fixes # (issue)

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Test Configuration:

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Copy link
Member

@cschuchardt88 cschuchardt88 left a comment

Choose a reason for hiding this comment

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

Can you fix the warning.

shargon
shargon previously approved these changes May 13, 2024
Copy link
Member

@shargon shargon left a comment

Choose a reason for hiding this comment

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

Could you test it @superboyiii ? LGTM

shargon
shargon previously approved these changes May 14, 2024
{
// If the corresponding version of the plugin is not found, get the latest version
jsonRelease = json.AsArray()
.OrderByDescending(s => Version.Parse(s["tag_name"]!.GetValue<string>()?.TrimStart('v')))
Copy link
Member

Choose a reason for hiding this comment

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

Version.Parse will crash if it's null. or we have a tag_name that isn't valid format.

@cschuchardt88
Copy link
Member

cschuchardt88 commented May 14, 2024

We should wait for neo-module merge to this repo before we update this!

@Jim8y
Copy link
Contributor Author

Jim8y commented May 14, 2024

We should wait for neo-module merge to this repo before we update this!

This has nothing to do with modules. I am just dealing with something else.

@cschuchardt88
Copy link
Member

We should wait for neo-module merge to this repo before we update this!

This has nothing to do with modules. I am just dealing with something else.

Maybe we should download from nuget. Because we moving download URL.

@cschuchardt88
Copy link
Member

We should wait for neo-module merge to this repo before we update this!

This has nothing to do with modules. I am just dealing with something else.

@Jim8y
Yes it does, we won't be using that repo anymore. So URL will change. But I do see your point.

if (latestVersion < pluginVersion)
{
// If the latest version is lower than the locally passed version, use https://github.com/neo-project/neo-modules/releases/latest/download to get the latest version
var latestDownloadUrl = $"https://github.com/neo-project/neo-modules/releases/latest/download/{pluginName}.zip";
Copy link
Member

Choose a reason for hiding this comment

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

We need to change the url to neo now, also for DownloadUrl, could you do it @Jim8y ?

…gins

* 'latest-plugins' of github.com:Jim8y/neo: (21 commits)
  fix: custom plugins won't shown by command `plugins` (neo-project#3269)
  COVERALL: Improve maintenance and readbility of some variables (neo-project#3248)
  Update nuget (neo-project#3262)
  [**Part-2**] Neo module/master fixes (neo-project#3244)
  Fix `dotnet pack` error (neo-project#3266)
  Fix and Update devcontainer.json to use Dockerfile  (neo-project#3259)
  Add optimization to template (neo-project#3247)
  Optimize plugin's models (neo-project#3246)
  fix CancelTransaction !signers.Any() (neo-project#3263)
  COVERALL: fix broken by changing report from lcov to cobertura (neo-project#3252)
  fix TraverseIterator count (neo-project#3261)
  Native: include DeprecatedIn hardfork into usedHardforks (neo-project#3245)
  [**Part-1**] `neo-module/master` (neo-project#3232)
  Make `ApplicationEngine.LoadContext` protection level `public` (neo-project#3243)
  improve parse method in neo-cli (neo-project#3204)
  Fix neo-project#3239 (neo-project#3242)
  Neo.CLI: enable hardforks for NeoFS mainnet (neo-project#3240)
  v3.7.4 (neo-project#3237)
  fix hardfork issues (neo-project#3234)
  Update src/Neo.CLI/CLI/MainService.Plugins.cs
  ...

# Conflicts:
#	src/Neo.CLI/CLI/MainService.Plugins.cs
.Where(s => s?["prerelease"]?.GetValue<bool>() == prerelease)
.Select(s =>
{
var tagName = s["tag_name"]?.GetValue<string>();
Copy link
Member

@cschuchardt88 cschuchardt88 May 25, 2024

Choose a reason for hiding this comment

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

Fix Dereference of a possibly null reference warning.

s?["tag_name"]?.GetValue<string>() i think s? was needed.

src/Neo.CLI/CLI/MainService.Plugins.cs Outdated Show resolved Hide resolved
cschuchardt88
cschuchardt88 previously approved these changes May 26, 2024
@Jim8y Jim8y requested a review from a team May 27, 2024 23:42
@Jim8y
Copy link
Contributor Author

Jim8y commented May 27, 2024

@superboyiii will need you to release a plugin version at this repo to test.

@cschuchardt88
Copy link
Member

@superboyiii will need you to release a plugin version at this repo to test.

He can release on his own repo and change the URL.

@superboyiii
Copy link
Member

I will check this

@superboyiii
Copy link
Member

@Jim8y Just a little worried about if someone use v3.7.4 CLI to install v3.7.5 Plugins, they may not want to upgrade CLI if there're only hotfixes on plugins.... If we consider the title situation, we should consider this situation as well. Or maybe we should release both cli and plugins everytime, it can also solve these.

Comment on lines +116 to +121
if (latestVersion < pluginVersion)
{
var latestDownloadUrl = $"https://github.com/neo-project/neo/releases/download/v{latestVersion}/{pluginName}.zip";
ConsoleHelper.Info($"Could not find the corresponding version, installing the latest: v{latestVersion}");
return await httpClient.GetStreamAsync(latestDownloadUrl);
}
Copy link
Member

@superboyiii superboyiii May 31, 2024

Choose a reason for hiding this comment

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

We need take care of (latestVersion > pluginVersion), why not always pick the latestVersion?

@cschuchardt88
Copy link
Member

I think with the release of 3.7.5 this should be good now. If no more issues in PR.

@Jim8y Resolve conflicts

shargon
shargon previously approved these changes Aug 12, 2024

if (jsonRelease != null)
{
var latestVersion = Version.Parse(jsonRelease["tag_name"]!.GetValue<string>()[1..]);
Copy link
Member

Choose a reason for hiding this comment

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

Why this 1? Start with v?

cschuchardt88
cschuchardt88 previously approved these changes Aug 14, 2024
src/Neo.CLI/CLI/MainService.Plugins.cs Outdated Show resolved Hide resolved

if (jsonRelease != null)
{
var latestVersion = Version.Parse(jsonRelease["tag_name"]!.GetValue<string>()[1..]);
Copy link
Member

Choose a reason for hiding this comment

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

Use Version.TryParse instead of Version.Parse?

Copy link
Member

@cschuchardt88 cschuchardt88 Aug 24, 2024

Choose a reason for hiding this comment

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

Version.TryParse is good

@shargon shargon dismissed stale reviews from cschuchardt88 and themself via 5b1e2fd August 23, 2024 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants