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

API design: Self-service: Install Apple App Store apps on macOS #22102

Open
wants to merge 88 commits into
base: main
Choose a base branch
from

Conversation

marko-lisica
Copy link
Member

lukeheath
lukeheath previously approved these changes Sep 16, 2024
docs/REST API/rest-api.md Outdated Show resolved Hide resolved
| ---- | ------- | ---- | -------------------------------------------- |
| software_title_id | integer | path | **Required**. The ID of the software title to download software package.|
| team_id | integer | query | **Required**. The team ID. Downloads a software package added to the specified team. |
| alt | integer | query | **Required**. If specified and set to "media", downloads the specified software package. |
Copy link
Member

Choose a reason for hiding this comment

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

What happens if you set it to something else?

Copy link
Member Author

Choose a reason for hiding this comment

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

@rachaelshaw We return this error:

image

Copy link
Member

Choose a reason for hiding this comment

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

What's the reason for including the alt parameter if it's required & only supports one value? Are we planning on changing this to be a "Get or download" endpoint in the future & making alt optional?

Copy link
Member Author

Choose a reason for hiding this comment

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

@rachaelshaw We decided in the past that we'll use alt=media for download. Ideally, it would be "Get or download", but we don't need "Get" in the UI in this case, so that's why we have alt required.

Copy link
Member

@rachaelshaw rachaelshaw left a comment

Choose a reason for hiding this comment

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

Small note about the self-service language in the YAML, and one more question

docs/Configuration/yaml-files.md Outdated Show resolved Hide resolved
docs/Contributing/API-for-contributors.md Show resolved Hide resolved
| ---- | ------- | ---- | -------------------------------------------- |
| software_title_id | integer | path | **Required**. The ID of the software title to download software package.|
| team_id | integer | query | **Required**. The team ID. Downloads a software package added to the specified team. |
| alt | integer | query | **Required**. If specified and set to "media", downloads the specified software package. |
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason for including the alt parameter if it's required & only supports one value? Are we planning on changing this to be a "Get or download" endpoint in the future & making alt optional?

Co-authored-by: Rachael Shaw <r@rachael.wtf>
@noahtalerman
Copy link
Member

Hey @marko-lisica just a heads up that this reference docs PR has merge conflicts.

I think we want to get those resolved before Rachael approves otherwise I think we'll have to request another review

Please let me know if I can help resolve them!

@marko-lisica
Copy link
Member Author

@noahtalerman I just resolved the conflict. @rachaelshaw When you get the chance, could you take one more look?

rachaelshaw
rachaelshaw previously approved these changes Sep 20, 2024
fleet-release
fleet-release previously approved these changes Sep 24, 2024
fleet-release
fleet-release previously approved these changes Sep 24, 2024
Comment on lines +544 to +547
- [Batch-apply packages](#batch-apply-packages)
- [Batch-apply App Store apps](#batch-apply-app-store-apps)
- [Get token to download package](#get-token-to-download-package)
- [Download package using a token](#download-package-using-a-token)
Copy link
Member

@noahtalerman noahtalerman Sep 24, 2024

Choose a reason for hiding this comment

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

@lucasmrod when we merged in reference docs for the 4.57 release, it looks like the batch-apply endpoints got moved to the REST API docs instead of the contributors docs (see here).

I moved them back. When you get the chance, can you please check to see if these changes look right?

Also, I moved the download endpoints here b/c I think they're only for the Fleet UI to use (#21341). The best practice download endpoint for the IT admin to use in automations is here: https://fleetdm.com/docs/rest-api/rest-api#download-package

Copy link
Member

Choose a reason for hiding this comment

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

I'm super confused. It seems we deliberately moved them from API for contributors to REST API? (here).

Copy link
Member

@noahtalerman noahtalerman Sep 24, 2024

Choose a reason for hiding this comment

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

Hmm, I see. I think up to @rachaelshaw.

Rachael re your comment here, are we changing our philosophy? That is, endpoints that fleetctl uses (not best practice for automation use cases) live in API for contributors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants