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

[WIP] 491 formal spec reorg #679

Closed
wants to merge 41 commits into from
Closed

[WIP] 491 formal spec reorg #679

wants to merge 41 commits into from

Conversation

kriswest
Copy link
Contributor

@kriswest kriswest commented Apr 27, 2022

resolves #491
resolves #482
resolves #483
prepares for but doesn't close (as page is disabled):

Reorganizes the FDC3 website navigation, introduces an implementations page, an abstract page for the standard and reactors standard content pages to reduce duplication and improve organization

@kriswest kriswest mentioned this pull request Apr 28, 2022
17 tasks
Copy link
Member

@robmoffat robmoffat left a comment

Choose a reason for hiding this comment

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

As a result a launcher can use a shortened URI construct "https://appd.foo.com/api/appd/v2/app1" to resolve the application data vs "https://appd.foo.com/api/appd/v2/app1@appd.foo.com".

Isn't there a potential here to collide with a REST endpoint for the appD?

@kriswest
Copy link
Contributor Author

kriswest commented May 11, 2022

Isn't there a potential here to collide with a REST endpoint for the appD?

This is in fact the REST GET endpoint for the appd. Ignore the v1 / v2 mis match in the URL, I put the v2 link in the copy to save updating it later, a different PR from the appD discussion group actually adds the v2 endpoints.

We should actually add a comment to the openapi spec about being able to use either the fully qualified app id or bare. I'll take care of that at some point.

@robmoffat
Copy link
Member

I presume you'll get Hugh's review as well before merging. I'm working with @maoo and @TheJuanAndOnly99 to try and get the preview server up and running so he can look at the complete version

@kriswest
Copy link
Contributor Author

I presume you'll get Hugh's review

Yep and ideally other maintainers as well, hence a preview would be very useful!

Copy link
Contributor

@hughtroeger hughtroeger left a comment

Choose a reason for hiding this comment

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

Overall looks good, some small comments, a few broken/stale links. Don't see any breaking changes to the spec.

@kriswest I'm happy to file a PR with these changes if you would like.

docs/intents/spec.md Outdated Show resolved Hide resolved
docs/intents/spec.md Outdated Show resolved Hide resolved
docs/api/spec.md Outdated Show resolved Hide resolved
docs/api/spec.md Outdated Show resolved Hide resolved
docs/api/spec.md Outdated Show resolved Hide resolved
docs/api/spec.md Outdated Show resolved Hide resolved
docs/api/spec.md Outdated Show resolved Hide resolved
docs/app-directory/overview.md Outdated Show resolved Hide resolved
docs/app-directory/overview.md Outdated Show resolved Hide resolved
{doc: 'use-cases/overview', label: 'Use Cases'},
//{page: 'implementations', label: 'Implementations'},
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want this commented out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes for now - its added but shouldn't be linked to/released until populated (and relationship to the FDC3 conformance program worked out). Separate PR raised to enable it:

docs/api/spec.md Outdated Show resolved Hide resolved
docs/api/spec.md Outdated Show resolved Hide resolved
docs/context/spec.md Outdated Show resolved Hide resolved
docs/intents/spec.md Outdated Show resolved Hide resolved
Co-authored-by: Hugh Troeger <troeger.hugh@gmail.com>
@kriswest
Copy link
Contributor Author

@greyseer256 many thanks for the review - changes all applied

@kriswest kriswest requested a review from hughtroeger May 12, 2022 09:25
Copy link
Contributor

@hughtroeger hughtroeger left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@kriswest
Copy link
Contributor Author

Closing to re-create with preview

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