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

Introduce metadata for all add-ons #3050

Merged
merged 8 commits into from
Jan 15, 2023
Merged

Conversation

J-N-K
Copy link
Member

@J-N-K J-N-K commented Jul 24, 2022

Closes #2058
Depends on #2994

This removes the binding.xml and replaces it with an addon.xml which allows to provide metadata for all add-ons. Also contains necessary changes in other components.

@J-N-K J-N-K force-pushed the feature-addonxml branch from fe597df to 69c1d71 Compare August 16, 2022 17:51
@J-N-K J-N-K added the enhancement An enhancement or new feature of the Core label Aug 18, 2022
@J-N-K J-N-K changed the title [WIP] Introduce metadata for all add-ons Introduce metadata for all add-ons Aug 18, 2022
@J-N-K J-N-K marked this pull request as ready for review August 18, 2022 15:57
@J-N-K J-N-K requested a review from a team as a code owner August 18, 2022 15:57
@lsiepel
Copy link
Contributor

lsiepel commented Aug 18, 2022

I don’t see a PR for updating the developer documentation / binding template etc. Am I overlooking something? I hope so.

@wborn
Copy link
Member

wborn commented Aug 19, 2022

The i18n-plugin also needs to be updated as it reads Binding XML info for generating translations. It will probably still compile locally because its bundle dependency still exists in your local and the OH snapshot repo.

@J-N-K J-N-K force-pushed the feature-addonxml branch from df18b8c to 5ff40dc Compare August 19, 2022 16:05
@J-N-K
Copy link
Member Author

J-N-K commented Aug 19, 2022

The i18n-maven-plugin changes are already part of this PR.

@wborn wborn added the awaiting other PR Depends on another PR label Aug 20, 2022
@wborn wborn added this to the 4.0 milestone Aug 20, 2022
@J-N-K J-N-K force-pushed the feature-addonxml branch from bdd6312 to c42eecf Compare August 21, 2022 17:23
@J-N-K J-N-K mentioned this pull request Dec 15, 2022
5 tasks
@wborn wborn removed the awaiting other PR Depends on another PR label Dec 19, 2022
@wborn wborn removed this from the 4.0 milestone Dec 19, 2022
@jlaur
Copy link
Contributor

jlaur commented Jan 1, 2023

What is the next step for this PR? Perhaps now would be a good time to get it finished so we have a full release cycle for testing, assuming it would make it into the first 4.0 milestone.

Before asking @J-N-K to resolve merge conflicts (also in related PR's) and preparing a PR for the Zigbee binding, perhaps a @openhab/core-maintainers could have a look and write some kind of statement about "conditional approval", so that Jan won't have to resolve conflicts and rebase too many times? @kaikreuzer - it seems this was suggested by you in #2058, so perhaps you could have a look and comment? 🙂

@J-N-K J-N-K force-pushed the feature-addonxml branch from aa66498 to d0c9b97 Compare January 1, 2023 14:18
@J-N-K J-N-K force-pushed the feature-addonxml branch 2 times, most recently from 99caab2 to e16aac6 Compare January 3, 2023 19:17
@kaikreuzer
Copy link
Member

Good point @jlaur! Yes, I'll have a look at it the next days (latest by the end of the week). Thanks @J-N-K for rebasing it already.

J-N-K added 3 commits January 14, 2023 21:14
Signed-off-by: Jan N. Klug <github@klug.nrw>
Signed-off-by: Jan N. Klug <github@klug.nrw>
Signed-off-by: Jan N. Klug <github@klug.nrw>
Signed-off-by: Jan N. Klug <github@klug.nrw>
@J-N-K
Copy link
Member Author

J-N-K commented Jan 14, 2023

Done

Copy link
Member

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

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

A few first (very small) findings - more to come tomorrow!

xstream.alias("name", NodeValue.class);
xstream.alias("description", NodeValue.class);
xstream.alias("author", NodeValue.class);
xstream.alias("service-id", NodeValue.class);
Copy link
Member

Choose a reason for hiding this comment

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

What has happened to the service-id?

Copy link
Member Author

Choose a reason for hiding this comment

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

Originally I planned to change it to config-id and later decided against that. I have reverted this changed (same answer below).

J-N-K added 2 commits January 15, 2023 00:52
Signed-off-by: Jan N. Klug <github@klug.nrw>
Signed-off-by: Jan N. Klug <github@klug.nrw>
Copy link
Member

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

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

Many thanks @J-N-K!
Looks all perfect to me - all I could find are afew remaining missed replacements in the Javadoc.

Signed-off-by: Jan N. Klug <github@klug.nrw>
@J-N-K
Copy link
Member Author

J-N-K commented Jan 15, 2023

Thanks, I also prepared the missing PR for zwave and zigged (which are quite straight-forward) and updated the PR in openhab-addons (which is also mostly copying and addition of the new add-ons like persistence). What is missing is a proper review of the webui-PR regarding the changes in MainUI.

Copy link
Member

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

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

What is missing is a proper review of the webui-PR regarding the changes in MainUI.

I can't really review the code, but I will test if it still works in general. If there are small things to correct, I guess @ghys can also comment/fix it later on.

So would you say that from your pov everything should then be ready to be merged today already?

@J-N-K
Copy link
Member Author

J-N-K commented Jan 15, 2023

I think it's fine now, I removed some leftovers in addons, otherwise I would say: Go! If something comes up I can fix it on short notice.

@kaikreuzer
Copy link
Member

Excellent, then let us go for it!

@kaikreuzer kaikreuzer merged commit 3d54ee5 into openhab:main Jan 15, 2023
@kaikreuzer kaikreuzer added this to the 4.0 milestone Jan 15, 2023
@J-N-K J-N-K deleted the feature-addonxml branch January 15, 2023 15:36
@J-N-K
Copy link
Member Author

J-N-K commented Jan 15, 2023

@kaikreuzer Can you upload the XSD so it is available for validation? I'll prepare a PR to change the static analysis.

@kaikreuzer
Copy link
Member

It is all in git. :-)
XSD uploaded here: openhab/website#387

Comment on lines +15 to +19
<xs:element name="author" type="xs:string" minOccurs="0">
<xs:annotation>
<xs:documentation>The organization maintaining the add-on (e.g. openHAB). Individual developer names should be avoided.</xs:documentation>
</xs:annotation>
</xs:element>
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for commenting code already merged, but thought it would be easiest for proper context. Should it be mentioned that it's not needed to use "openHAB" as author here? At least it seems none of the add-ons in the openhab-addons repository have this, and the skeleton script doesn't create it.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe it would be the right time to drop this field now altogether - we more or less only kept it for backward-compatibility, but as we are breaking this now anyhow, I think we could remove it from the XSD.

Copy link
Member

Choose a reason for hiding this comment

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

Or are we using it for Marketplace entries, @ghys?

Copy link
Member

Choose a reason for hiding this comment

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

The marketplace add-on service will build the add-on with what Discourse returns as the author of the topic as the add-on author:

return Addon.create(id).withType(type).withContentType(contentType).withImageLink(topic.imageUrl)
.withAuthor(author).withProperties(properties).withLabel(title).withInstalled(installed)
.withMaturity(maturity).withCompatible(compatible).withLink(link).build();

and the Karaf add-on service will put the constant "openHAB" in that field (and set it as "verified"):

Addon.Builder addon = Addon.create(id).withContentType(ADDONS_CONTENT_TYPE).withType(type)
.withVersion(feature.getVersion()).withAuthor(ADDONS_AUTHOR, true).withInstalled(isInstalled);

The author and the checkmark are displayed in the UI, notably in the information tables in each add-on page. For "verified" add-ons it's also displayed in the list.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @ghys, this means that this field is definitely deprecated then.
@J-N-K Would you agree that this is a good moment to remove it from the XSD then?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API breaking enhancement An enhancement or new feature of the Core
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Introduce add-on metadata
6 participants