-
Notifications
You must be signed in to change notification settings - Fork 63
App store API updates #2497
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
App store API updates #2497
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2497 +/- ##
==========================================
- Coverage 79.23% 79.19% -0.05%
==========================================
Files 670 670
Lines 54182 54225 +43
Branches 734 734
==========================================
+ Hits 42930 42941 +11
- Misses 11172 11204 +32
Partials 80 80
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jmthomas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. What does this actually look like in the gemspec? Something like:
s.openc3_cosmos_minimim_version = '6.0.0'
Does it support a version like 6.0 or just 6?
|
@jmthomas You set it with the other app store properties like this: s.metadata = {
"openc3_cosmos_minimum_version" => "6.9.0",
"openc3_store_title" => "Video Player",
"openc3_store_description" => "This plugin provides COSMOS with an integrated video player capable of playing back most common video file types, as well as HLS streams. Included is the top-level tool and a widget that can be used in TlmViewer screens.",
"openc3_store_keywords" => "video, streaming",
"openc3_store_image" => "public/store_img.png"
}It has to be a valid semver string, so you have to specify all three digits (6.0.0). It does support the |
|
Do not merge until after the next App Store deployment (ETA: 11/24 - 11/26). The app store changes that this depends on have been merged, but the infra needed for their prod deployment has not been set up yet. |
| original_filename = File.basename(store_data['gem_url']) | ||
|
|
||
| # Try to find the correct hostname (in case it's localhost and needs to be host.docker.internal) | ||
| adjusted_gem_url = check_localhost_reachability(store_data['gem_url'], params[:store_id]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would a app store author publish a gem with a URL of localhost? That doesn't really make sense right? Don't we require these to be available at Rubygems or some internal repo like artifactory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Being able to self-host the store is one of its requirements, so gems might be at localhost
5d1a022 to
d7bc517
Compare
d7bc517 to
58363ce
Compare
|
Added API key support for Enterprise/allowlist plugins and caught this up with recent app store changes. Ready for re-review |
jmthomas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good but I'm not sure why I'm getting the "Plugin Store Error" when I try to connect.
| api_key_setting = OpenC3::SettingModel.get(name: 'store_api_key', scope: 'DEFAULT') | ||
| api_key = api_key_setting['data'] if api_key_setting | ||
|
|
||
| test_url = "http://#{uri.host}:#{uri.port}/api/v1.1/cosmos_plugins/#{store_id}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is for deploying your own app store? How / where will this be documented?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be documented when we open source the app store
|
|
||
| const settingName = 'store_url' | ||
| const SETTING_NAME = 'store_url' | ||
| const DEFAULT_STORE_URL = 'https://store.openc3.com' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I run this branch I get the following when browsing the app store: Error contacting plugin store at https://store.openc3.com (status: 404)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works now that the app store is launched
| image_url: String, // Set for app store plugins | ||
| licenses: Array, | ||
| // rating: Number, | ||
| // downloads: Number, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is with all the commented out fields here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Features that were in the proof of concept, and will be implemented for real later
| settings with the | ||
| <v-icon icon="mdi-cog" size="small" /> icon and paste your API key into | ||
| the API key field. | ||
| </v-card-text> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this all implemented in the App Store?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
| 'name' => @name, | ||
| 'variables' => @variables, | ||
| 'plugin_txt_lines' => @plugin_txt_lines, | ||
| 'minimum_cosmos_version' => @minimum_cosmos_version, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need a migration to support adding this field or I assume if it is nil we just ignore it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's nil, we ignore it (assume minimum version of 5.0.0)
d29b01c to
34c4275
Compare
6952b5b to
95ab110
Compare
See also: https://github.com/OpenC3/app-store/pull/41