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

Allow changing log-levels for add-ons #1452

Merged
merged 1 commit into from
Jan 22, 2023
Merged

Conversation

J-N-K
Copy link
Member

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

Closes #1351

Depends on openhab/openhab-core#3045

As discussed the gear wheel is now shown on each add-on page if either a config-description URI or a set of logger-packages is found.

Signed-off-by: Jan N. Klug github@klug.nrw

@relativeci
Copy link

relativeci bot commented Jul 17, 2022

Job #716: Bundle Size — 15.97MiB (+0.03%).

e613a28(current) vs f735f45 main#715(baseline)

Metrics (4 changes)
                 Current
Job #716
     Baseline
Job #715
Initial JS 1.73MiB(~+0.01%) 1.73MiB
Initial CSS 608.52KiB 608.52KiB
Cache Invalidation 90.48% 90.48%
Chunks 218 218
Assets 688 688
Modules 2011(-0.1%) 2013
Duplicate Modules 108 108
Duplicate Code 1.8%(+1.69%) 1.77%
Packages 133(-1.48%) 135
Duplicate Packages 15 15
Total size by type (2 changes)
                 Current
Job #716
     Baseline
Job #715
CSS 856.56KiB 856.56KiB
Fonts 1.08MiB 1.08MiB
HTML 1.23KiB 1.23KiB
IMG 140.74KiB 140.74KiB
JS 9.04MiB (+0.35%) 9.01MiB
Media 295.6KiB 295.6KiB
Other 4.59MiB (-0.58%) 4.61MiB

View job #716 reportView main branch activity

@J-N-K
Copy link
Member Author

J-N-K commented Jul 17, 2022

Bildschirmfoto 2022-07-17 um 13 43 52

@ghys
Copy link
Member

ghys commented Jul 24, 2022

Tried it and I don't see the configuration for the Kodi binding anymore:

image

image

The reason is that the configDescriptionURI is empty (3.3.0 Release) in /rest/addons/binding-kodi:

image

Since it appears to work in your screenshot above, I would suspect that the configDescriptionURI can eventually appear in the /rest/addons response, but in this case it does not? Otherwise it might be safer to still try to retrieve it from /rest/bindings if is identified as a binding, like before.

@J-N-K
Copy link
Member Author

J-N-K commented Jul 24, 2022

Did you try with a snapshot after openhab/openhab-core#3045 was merged?

{
  "id": "binding-kodi",
  "label": "Kodi Binding",
  "version": "3.4.0.SNAPSHOT",
  "compatible": true,
  "contentType": "application/vnd.openhab.feature;type=karaf",
  "link": "https://www.openhab.org/addons/bindings/kodi/",
  "author": "openHAB",
  "verifiedAuthor": true,
  "installed": true,
  "type": "binding",
  "configDescriptionURI": "binding:kodi",
  "keywords": "",
  "countries": "",
  "connection": "",
  "properties": {},
  "loggerPackages": [
    "org.openhab.binding.kodi"
  ]
}

@ghys
Copy link
Member

ghys commented Jul 24, 2022

Ah, my bad, I thought the 3.3.0 core was sufficient, I missed you mentioned the dependency to openhab/openhab-core#3045. Will do.

@ghys
Copy link
Member

ghys commented Jul 24, 2022

Also in the /settings/things/add page where you select the binding:

image

The gear icon link to configure the binding (if it has a configDescription) needs to be updated to the new route.
It's important because it's AFAIK still the only way to configure a binding that wasn't added from the distribution.
Also it could make sense to display this icon regardless of whether a configuration exists now since the screen also includes the log level section.

@J-N-K
Copy link
Member Author

J-N-K commented Jul 24, 2022

Currently the "old route" will still work. This will change with openhab-core#3050 but that's more at the beginning of next year. There I'm also working on reporting manually installed add-ons (i.e. in addons folder) in the REST API, to we can drop the configuration at that place.

@ghys
Copy link
Member

ghys commented Jul 29, 2022

Yes the route works but it fails to find the add-on it should pull the config from:

When clicking that gear icon besides the Hue entry it navigates to /settings/addons/hue/config which fails to render:

image

This is due to a GET /rest/addons/hue which results in a 404 error (the former call was /rest/bindings/hue).
Now it could be tricky to translate the bindingId (hue) we get from the binding list page into an add-on id. It could be binding-hue or marketplace:{something} or yet something else entirely depending on the underlying add-on service.
I have no good solution for this, and I can only offer to keep the old binding-config.vue along with a separate route in routes.js for this page. It will not feature the log level sections of the new add-on configuration page, but these can still be accessed from the add-on store.

Copy link
Member

@ghys ghys left a comment

Choose a reason for hiding this comment

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

Alright, great. Quite unfortunate to have to have 2 separate pages to configure the bindings, but it's only a little overhead and it solves other problems until all add-ons can be configured, so "oh well".

Last cosmetic suggestions below on the section titles in the add-on config page to make them look better. They are also bigger because there might be config description groups in the configuration, which would render group titles on the same level.

image

image

Copy link
Member

@ghys ghys left a comment

Choose a reason for hiding this comment

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

OK, LGTM!
Since it's still technically a draft, please mark it ready when you are.

@J-N-K
Copy link
Member Author

J-N-K commented Aug 9, 2022

I would prefer to keep user-facing changes for beginning of next year (i.e. after release), otherwise we'll have the same discussion as in https://github.com/orgs/openhab/teams/architecture-council/discussions/15 again: Not enough visible changes to justify a change in Java version.

@J-N-K J-N-K marked this pull request as ready for review January 2, 2023 12:05
@J-N-K J-N-K requested a review from a team as a code owner January 2, 2023 12:05
@@ -204,6 +204,10 @@ export default {
}
},
computed: {
showConfig () {
console.error('configUri' + this.addon.configDescriptionURI)
Copy link
Member

Choose a reason for hiding this comment

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

I had another check: can you remove this debugging leftover?

@ghys
Copy link
Member

ghys commented Jan 16, 2023

This now needs rebasing and fixing the conflicts since #1468 was merged.

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

J-N-K commented Jan 17, 2023

Done. However, to work a regression core needs to be fixed: openhab/openhab-core#3322

@hmerk
Copy link

hmerk commented Jan 21, 2023

As openhab/openhab-core#3322 was merged 3 days ago, could this one be merged as well ?
Would love to try this feature with one of the next SNAPSHOTs ....

@ghys ghys merged commit d5da973 into openhab:main Jan 22, 2023
@J-N-K J-N-K deleted the feature-loglevel branch January 22, 2023 07:23
@hmerk
Copy link

hmerk commented Jan 22, 2023

@ghys Thanks Yannick !

@hmerk
Copy link

hmerk commented Jan 22, 2023

@J-N-K Great job, working as expected 👍

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

Successfully merging this pull request may close these issues.

Allow changing add-on log-levels
3 participants