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

Fix a few issues with the plugin manifest #3164

Merged
merged 8 commits into from Jun 4, 2020
Merged

Fix a few issues with the plugin manifest #3164

merged 8 commits into from Jun 4, 2020

Conversation

dkanada
Copy link
Member

@dkanada dkanada commented May 24, 2020

This removes the last of the annoying properties from the version and package info classes. The installation is now used internally to collect data from both classes, but at some point we could also start using a tuple instead.

@dkanada dkanada added this to In progress in Release 10.6.0 via automation May 24, 2020
@dkanada
Copy link
Member Author

dkanada commented May 24, 2020

@mark-monteiro this should resolve the issue in #2951 by reverting the version to a string in the serialization object and parsing it as a version for the class that gets passed around for the install.

@sonarcloud
Copy link

sonarcloud bot commented May 24, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 6 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

Copy link
Member

@mark-monteiro mark-monteiro left a comment

Choose a reason for hiding this comment

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

In general this looks fine but I couldn't test because the web UI for installing plugins is broken. Is there a web PR that needs to go along with this?

image

Selecting one of the 'undefined' options and clicking install fails and gives this error in the console:

Error: null updateClass apiClient.js:1368:18

Emby.Server.Implementations/Updates/InstallationManager.cs Outdated Show resolved Hide resolved
@dkanada
Copy link
Member Author

dkanada commented May 24, 2020

Looks like I'll have to do another pass through web as well, this was mainly to remove the last few duplicate properties and fix the serialization issues.

@mark-monteiro
Copy link
Member

Looks like I'll have to do another pass through web as well, this was mainly to remove the last few duplicate properties and fix the serialization issues.

Yeah, the errors in the web client did not appear to be related to this PR. But it would be nice to have the web side working to test this.

@Artiume
Copy link
Contributor

Artiume commented May 24, 2020

general this looks fine but I couldn't test because the web UI for installing plugins is broken. Is there a web PR that needs to go along with this?

@barronpm and I saw a similar issue in the User DB migrations for User Authentication

@barronpm
Copy link
Member

The cause of the error @Artiume mentioned was accidentally adding the same assembly to the Enumerable returned by ApplicationHost.GetComposablePartAssemblies, but this doesn't look the same, as all of the options are "undefined" for you

@mark-monteiro
Copy link
Member

I believe the issue I had was fixed here: https://github.com/jellyfin/jellyfin-web/pull/1291/files
Just need to find some time to come back to this to test it again

@dkanada
Copy link
Member Author

dkanada commented May 26, 2020

@mark-monteiro there's still one more PR to merge in web updating the API client before this should be working again.

EDIT: That PR was merged and this should ideally be working now.

@dkanada
Copy link
Member Author

dkanada commented May 27, 2020

Versions seem to be fine, but the installation is failing due to changes from the API client migration.

@crobibero
Copy link
Member

Looks like InstallationInfo.SourceUrl needs to be set in Emby.Server.Implementations\Updates\InstallationManager.cs L220

Co-authored-by: Vasily <JustAMan@users.noreply.github.com>
Co-authored-by: Cody Robibero <cody@robibe.ro>
@dkanada dkanada requested a review from crobibero June 3, 2020 06:24
Release 10.6.0 automation moved this from In progress to Review in progress Jun 3, 2020
dkanada and others added 2 commits June 4, 2020 03:18
Co-authored-by: Cody Robibero <cody@robibe.ro>
@crobibero
Copy link
Member

Publish failing due to following errors:

EntryPoints/ServerEventNotifier.cs(91,13): error CS4014: Because this call is not awaited, execution of the current method continues before the call is completed. Consider applying the 'await' operator to the result of the call. [/home/vsts/work/1/s/Emby.Server.Implementations/Emby.Server.Implementations.csproj]
EntryPoints/ServerEventNotifier.cs(89,28): error CS1998: This async method lacks 'await' operators and will run synchronously. Consider using the 'await' operator to await non-blocking API calls, or 'await Task.Run(...)' to do CPU-bound work on a background thread. [/home/vsts/work/1/s/Emby.Server.Implementations/Emby.Server.Implementations.csproj]
EntryPoints/ServerEventNotifier.cs(96,13): error CS4014: Because this call is not awaited, execution of the current method continues before the call is completed. Consider applying the 'await' operator to the result of the call. [/home/vsts/work/1/s/Emby.Server.Implementations/Emby.Server.Implementations.csproj]
EntryPoints/ServerEventNotifier.cs(94,28): error CS1998: This async method lacks 'await' operators and will run synchronously. Consider using the 'await' operator to await non-blocking API calls, or 'await Task.Run(...)' to do CPU-bound work on a background thread. [/home/vsts/work/1/s/Emby.Server.Implementations/Emby.Server.Implementations.csproj]
EntryPoints/ServerEventNotifier.cs(101,13): error CS4014: Because this call is not awaited, execution of the current method continues before the call is completed. Consider applying the 'await' operator to the result of the call. [/home/vsts/work/1/s/Emby.Server.Implementations/Emby.Server.Implementations.csproj]
EntryPoints/ServerEventNotifier.cs(99,28): error CS1998: This async method lacks 'await' operators and will run synchronously. Consider using the 'await' operator to await non-blocking API calls, or 'await Task.Run(...)' to do CPU-bound work on a background thread. [/home/vsts/work/1/s/Emby.Server.Implementations/Emby.Server.Implementations.csproj]
EntryPoints/ServerEventNotifier.cs(121,13): error CS4014: Because this call is not awaited, execution of the current method continues before the call is completed. Consider applying the 'await' operator to the result of the call. [/home/vsts/work/1/s/Emby.Server.Implementations/Emby.Server.Implementations.csproj]
EntryPoints/ServerEventNotifier.cs(119,28): error CS1998: This async method lacks 'await' operators and will run synchronously. Consider using the 'await' operator to await non-blocking API calls, or 'await Task.Run(...)' to do CPU-bound work on a background thread. [/home/vsts/work/1/s/Emby.Server.Implementations/Emby.Server.Implementations.csproj]

@dkanada
Copy link
Member Author

dkanada commented Jun 3, 2020

I see you've been enabling more errors 😆 I'll fix them now.

Release 10.6.0 automation moved this from Review in progress to Reviewer approved Jun 3, 2020
@dkanada dkanada merged commit 44eebd7 into jellyfin:master Jun 4, 2020
Release 10.6.0 automation moved this from Reviewer approved to Done Jun 4, 2020
@dkanada dkanada deleted the install-plugin branch June 4, 2020 05:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

8 participants