Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
API design: Fleet-maintained apps for macOS #21801
Changes from 7 commits
8295ad9
27d0734
84207ac
b2de7fa
9e6b563
f0aa19e
6293fc3
97b7fb1
e1fda54
f41a3b8
b5063f9
46aada2
484446a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 isrelease
, and for VSCode it would bestable
).(side-note: I think this should be called
filename
without underscore? We have some API parameters that accept a filename and we usefilename
- 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 havingrelease
orstable
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.
Gotcha, makes sense.
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
inhttps://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, seefleet/server/mdm/maintainedapps/apps.json
Line 1 in fa62b4c
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.Could we simplify that, so
filename
is optional in thisapps.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.
Absolutely! Makes sense to me.
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.
@mna We decided to cut
filename
and usename
instead. See Figma 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.
@marko-lisica sounds good, thanks!