-
Notifications
You must be signed in to change notification settings - Fork 473
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
API design: Fleet-maintained apps for macOS #21801
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## docs-v4.57.0 #21801 +/- ##
================================================
+ Coverage 63.59% 63.67% +0.08%
================================================
Files 1498 1498
Lines 118959 118996 +37
Branches 3503 3488 -15
================================================
+ Hits 75647 75769 +122
+ Misses 36465 36374 -91
- Partials 6847 6853 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Looks good! One question a parameter.
@georgekarrv Would be good to get your eyes on this.
docs/REST API/rest-api.md
Outdated
|
||
List available Fleet library apps. | ||
|
||
`GET /api/v1/fleet/software/fleet_library_apps` |
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.
Dev note:
List only apps that haven't added to a team yet.
- If app is added and new version is released later, we still don't want to show that app in the list.
- If user uploaded package, we want to exclude that app from the list as well
Co-authored-by: Noah Talerman <47070608+noahtalerman@users.noreply.github.com>
docs/REST API/rest-api.md
Outdated
"fleet_maintained_app": { | ||
"id": 1, | ||
"name": "1Password", | ||
"file_name": "1Password-8.10.44-aarch64.zip", |
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.
@marko-lisica The filename seems to be the installer's filename from this example. We don't have this info explicitly in the brew API, should I fill this with just the last section of the installer's URL? In most case this would give a sensible filename like what you have here, but for some apps (like WhatsApp: https://web.whatsapp.com/desktop/mac_native/release/?version=2.24.16.80&extension=zip&configuration=Release&branch=relbranch
), the filename would end up a bit weird (in this case, the last path segment is release
, and for VSCode it would be stable
).
(side-note: I think this should be called filename
without underscore? We have some API parameters that accept a filename and we use filename
- or at least that's the field name for the name of the file in uploads)
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.
Hey @mna, is there any other way to retrieve filename
? Could we make it if the URL doesn't end with extensions like: .dmg, .zip, .pkg, etc. then Fleet tries to download a file to get the name? Is that too complex? I think having release
or stable
as filename could be confusing.
I agree, we should be consistent and use filename
without _
. I'll update PR.
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.
We cannot download it at the time we process the brew metadata, as this cron job only calls the JSON API, it does not start downloading the installers (we do this only when adding the maintained app to a team).
I'm curious though - what is the purpose of that field besides a light (non-critical) piece of information? Given a URL like WhatsApp , the filename of the installer could be whatever we want it to be (i.e. this is just a URL that sends some bytes, we can save those bytes under whatever filename we want). I'm thinking we could just generate a filename from the app name and installer type in this case (e.g. "whatsapp.dmg").
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.
@mna I think I added it so users understand what will be added to Fleet. Is it .pkg, .dmg, or something else. They'll see the $INSTALLER_PATH variable in the install script, it will help understand what's behind that variable.
Since you brought this up, I think we should probably remove it or convey information that the installer will be downloaded and uploaded to Fleet server in a different way. I'm going to bring this to design review today for expedited drafting.
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.
I added it so users understand what will be added to Fleet. Is it .pkg, .dmg, or something else. They'll see the $INSTALLER_PATH variable in the install script, it will help understand what's behind that variable.
Gotcha, makes sense.
I'm going to bring this to design review today for expedited drafting.
Sounds good, not a blocker for the moment but of course the sooner we can clarify, the better. Thanks!
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.
@marko-lisica correct, yes we (will) have a JSON file that describes the list of maintained apps, (e.g. it contains the "brew identifier" - the part to use in the API call e.g. 1password
in https://formulae.brew.sh/api/cask/1password.json
), the bundle identifier associated with this app (since we can't get it from brew) and the "installer format" which is an arbitrary string that defines how the installer is bundled, and comes from your research in the google doc (e.g. "dmg:app"). At least that's the plan at the moment, see
[ |
Maybe we could add the filename as part of this hard-coded JSON file? Only thing is if we want to include the version as part of the filename, that part is dynamic (we could make the filename some kind of template string that we fill with brew data). But that's a possibility!
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.
I didn't know that we'd hardcode installer_format
, but it makes sense.
Maybe we could add the filename as part of this hard-coded JSON file? Only thing is if we want to include the version as part of the filename, that part is dynamic (we could make the filename some kind of template string that we fill with brew data). But that's a possibility!
Could we simplify that, so filename
is optional in this apps.json
, and we provide it only for apps that have "weird" URLs (WhatsApp and VS Code)? Since we'll be defining apps that will appear in the Fleet-maintained tab, and we committed that we'll test it the first time to make sure, install/uninstall scripts work, we can do that as part of the process of adding new apps.
I think if we want accurate name we'll need it dynamic (template string that we fill with brew data)
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.
Could we simplify that, so filename is optional in this apps.json, and we provide it only for apps that have "weird" URLs (WhatsApp and VS Code)?
Absolutely! Makes sense to me.
I think if we want accurate name we'll need it dynamic (template string that we fill with brew data)
Allright, so we want the version in there and the extension to reflect its format, correct?
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.
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.
@marko-lisica sounds good, thanks!
API design for: #18865
New PR: #22552