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

Update the Application schema #437

Merged
merged 9 commits into from
Jan 18, 2022
Merged

Conversation

ggeorgievx
Copy link
Member

@ggeorgievx ggeorgievx commented Aug 2, 2021

Resolves #314

  • Modified the Application schema instead of introducing a general, host independent, fdc3.general.app.manifest manifest type describing an application, as agreed inside of Introduce a general, host independent, fdc3.general.app.manifest manifest type describing an application #314:
    • Removed the manifestType property
    • Removed the manifest property
    • Introduced a new required type property
    • Introduced a new details property
    • Introduced a new hostManifests property (a mapping from host string to host specific application manifest object or URI instead of an array)
  • Added an example Application definition
  • Added a test environment for the specification and example

Notes:

  • Valid values for the type property are browser and host - host would allow application providers to serve applications other than browser ones
  • The details are not marked as required as they are only needed when the application type is browser. Host type applications should use the hostManifests object for all application details.
  • The hostManifests is a mapping from host string to host specific application manifest object or URI (added URI for better backwards compatibility as some vendors used URI manifests)
  • A GitHub workflow could be created for the app-directory's test script

@ggeorgievx ggeorgievx added enhancement New feature or request app-directory labels Aug 2, 2021
@ggeorgievx ggeorgievx added this to the 2.0 milestone Aug 2, 2021
@ggeorgievx ggeorgievx force-pushed the feat/update-application-schema branch from e0440f9 to 4ae6e31 Compare August 2, 2021 12:40
Copy link

@thorsent thorsent left a comment

Choose a reason for hiding this comment

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

I added in some suggested changes around typos and phrasing. Otherwise this looks really good.

src/app-directory/specification/appd.yaml Outdated Show resolved Hide resolved
src/app-directory/specification/appd.yaml Outdated Show resolved Hide resolved
src/app-directory/specification/appd.yaml Outdated Show resolved Hide resolved
src/app-directory/specification/appd.yaml Outdated Show resolved Hide resolved
src/app-directory/specification/appd.yaml Outdated Show resolved Hide resolved
ggeorgievx and others added 5 commits September 23, 2021 16:20
Co-authored-by: Terry Thorsen <terry@chartiq.com>
Co-authored-by: Terry Thorsen <terry@chartiq.com>
Co-authored-by: Terry Thorsen <terry@chartiq.com>
Co-authored-by: Terry Thorsen <terry@chartiq.com>
Co-authored-by: Terry Thorsen <terry@chartiq.com>
@kriswest kriswest mentioned this pull request Oct 22, 2021
11 tasks
@kriswest kriswest mentioned this pull request Nov 15, 2021
10 tasks
Copy link
Contributor

@rikoe rikoe left a comment

Choose a reason for hiding this comment

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

@kriswest @ggeorgievx @mattjamieson is it okay to merge this as in interrim change on the road to 2.0, even though discussions around AppD are still ongoing? I am happy to do so, if you are.

@ggeorgievx
Copy link
Member Author

ggeorgievx commented Dec 21, 2021

@rikoe Thank you, Riko. I also don't see an issue with having this merged as there aren't other issues/PRs that conflict with it/offer alternatives. The big one (#468) is still under active discussion and if it were to be voted upon and merged would come with drastic changes anyway.

@kriswest
Copy link
Contributor

I also don't see an issue with having this merged as there aren't other issues/PRs that conflict with it/offer alternatives. The big one (#468) is still under active discussion and if it were to be voted upon and merged would come with drastic changes anyway.

I concur. I fully expect to have a further debate on #468 early in the new year + about how to improve appD further.

I've just realised I have a few small review comments pending, I'll submit those now as I think they need dealing with before we merge.

Copy link
Contributor

@kriswest kriswest left a comment

Choose a reason for hiding this comment

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

Several of my comments have already been dealt with however I think we should adjust the examples so that we're not dependent on external applications/domains + a changelog entry is needed.

src/app-directory/specification/appd.yaml Outdated Show resolved Hide resolved
src/app-directory/specification/appd.yaml Outdated Show resolved Hide resolved
src/app-directory/specification/appd.yaml Outdated Show resolved Hide resolved
src/app-directory/specification/test/index.js Outdated Show resolved Hide resolved
ggeorgievx and others added 2 commits December 21, 2021 21:10
@ggeorgievx
Copy link
Member Author

Several of my comments have already been dealt with however I think we should adjust the examples so that we're not dependent on external applications/domains + a changelog entry is needed.

@kriswest Thank you for the feedback, Kris! I just updated the example application to FDC3 Workbench and also noted all changes inside of the changelog. Please let me know if you have any additional comments/feedback.

Comment on lines 1 to 29
module.exports = {
appId: 'fdc3-workbench',
name: 'FDC3 Workbench',
type: 'browser',
details: {
url: 'https://fdc3.finos.org/toolbox/fdc3-workbench/'
},
hostManifests: {
Glue42: {
type: 'window',
icon: 'https://fdc3.finos.org/docs/assets/fdc3-logo.png',
details: {
height: 640,
width: 560,
left: 120,
top: 120,
mode: 'tab',
allowChannels: true,
loader: {
enabled: true,
hideOnLoad: true
}
},
customProperties: {
folder: 'FDC3 Toolbox'
}
}
}
};
Copy link
Contributor

Choose a reason for hiding this comment

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

The example might as well be exhaustive so I've populated the rest of the optional fields. I've also tweaked it to be valid JSON in case someone needs to copy/paste or we want to switch the example to a JSON file in future. Finally, I've added the Finsemble config example.

@ggeorgievx will need you to run this through the test to make sure it still validates.

Suggested change
module.exports = {
appId: 'fdc3-workbench',
name: 'FDC3 Workbench',
type: 'browser',
details: {
url: 'https://fdc3.finos.org/toolbox/fdc3-workbench/'
},
hostManifests: {
Glue42: {
type: 'window',
icon: 'https://fdc3.finos.org/docs/assets/fdc3-logo.png',
details: {
height: 640,
width: 560,
left: 120,
top: 120,
mode: 'tab',
allowChannels: true,
loader: {
enabled: true,
hideOnLoad: true
}
},
customProperties: {
folder: 'FDC3 Toolbox'
}
}
}
};
module.exports = {
"appId": "fdc3-workbench",
"name": "fdc3-workbench",
"title": "FDC3 Workbench",
"description": "Development and test tool for FDC3 desktop agents and apps",
"version": "1.0.0",
"tooltip": "FDC3 Workbench",
"icons": [
{
"url": "http://fdc3.finos.org/toolbox/fdc3-workbench/fdc3-icon-256.png"
}
],
"images": [
{
"url": "https://fdc3.finos.org/docs/assets/fdc3-logo.png",
"tooltip": "FDC3 logo"
}
],
"contactEmail": "fdc3@finos.org",
"supportEmail": "fdc3-maintainers@finos.org",
"publisher": "FDC3",
"intents": [
{
"name": "ViewChart",
"displayName": "View Chart",
"contexts": ["fdc3.instrument"]
}
],
"type": "browser",
"details": {
"url": "https://fdc3.finos.org/toolbox/fdc3-workbench/"
},
"hostManifests": {
"Glue42": {
"type": "window",
"icon": "https://fdc3.finos.org/docs/assets/fdc3-logo.png",
"details": {
"height": 640,
"width": 560,
"left": 120,
"top": 120,
"mode": "tab",
"allowChannels": true,
"loader": {
"enabled": true,
"hideOnLoad": true
}
},
"customProperties": {
"folder": "FDC3 Toolbox"
}
},
"Finsemble": {
"window": {
"left": 120,
"top": 120,
"width": 800,
"height": 750,
"options": {
"minWidth": 75
}
},
"foreign": {
"components": {
"App Launcher": {
"launchableByUser": true
},
"Toolbar": {
"iconURL": "http://fdc3.finos.org/toolbox/fdc3-workbench/fdc3-icon-256.png"
},
"Window Manager": {
"FSBLHeader": true,
"persistWindowState": true
}
}
},
"interop": {
"autoConnect": true
}
}
}
};

Copy link
Contributor

@kriswest kriswest left a comment

Choose a reason for hiding this comment

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

Although I think there will definitely be further changes, I think this PR can be merged as its an improvement on the current state.

@ggeorgievx I've suggested a change to the example that I'd appreciate you integrating (and giving a quick test) before we merge.

@ggeorgievx
Copy link
Member Author

Although I think there will definitely be further changes, I think this PR can be merged as its an improvement on the current state.

@ggeorgievx I've suggested a change to the example that I'd appreciate you integrating (and giving a quick test) before we merge.

@kriswest Thank you, Kris! I updated the example and ran the test successfully. We should be ready to go on this one.

Copy link
Contributor

@kriswest kriswest left a comment

Choose a reason for hiding this comment

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

LGTM

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

Successfully merging this pull request may close these issues.

Introduce a general, host independent, fdc3.general.app.manifest manifest type describing an application
5 participants