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

Adds a provider lookup for sitemaps #2162

Closed
wants to merge 12 commits into from
Closed

Adds a provider lookup for sitemaps #2162

wants to merge 12 commits into from

Conversation

bigbasec
Copy link
Contributor

This adds a sitemap provider lookup to the proxy service.

It's one part of a 2 part fix for issues :
openhab/openhab-webui#763
#2154
openhab/openhab-webui#745

Signed-off-by: Brian Homeyer <bhomeyer@gmail.com>
@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/images-video-sitemaps-and-proxy/115677/4

private final Logger logger = LoggerFactory.getLogger(ProxyServletService.class);

private final List<SitemapProvider> sitemapProviders = new ArrayList<>();

public interface SitemapSubscriptionCallback {
Copy link
Member

Choose a reason for hiding this comment

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

What is this interface good for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Leftover from testing. Removed.

}

@Override
public void modelChanged(String modelName, EventType type) {
Copy link
Member

Choose a reason for hiding this comment

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

Why does the class implement ModelRepositoryChangeListener? This method does not seem to do anything besides doing a debug log statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Leftover from testing. Removed.

Copy link
Contributor Author

@bigbasec bigbasec left a comment

Choose a reason for hiding this comment

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

All suggestions have been implemented. Thank you!

@kaikreuzer
Copy link
Member

Thanks for your updates.
Please run

mvn spotless:apply

as adviced by the failing Jenkins build.

@bigbasec
Copy link
Contributor Author

bigbasec commented Feb 4, 2021

Thanks for your updates.
Please run

mvn spotless:apply

as adviced by the failing Jenkins build.

I actually didn't do that because on a clean checkout the first time you run that it'll change a number of files that aren't really being changed. Something with the way Eclipse does what it does I would suspect. If I do that now and push those changes this modification goes from 1 file to 11, which is not the desired commit.

I'm switching to VS Code, hopefully have better results.

@kaikreuzer
Copy link
Member

it'll change a number of files that aren't really being changed

Then only commit the change that concerns the class you are touching here and all should be fine.

Removing old sitemap reference
Copy link
Contributor Author

@bigbasec bigbasec left a comment

Choose a reason for hiding this comment

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

Removed as per review.

@bigbasec
Copy link
Contributor Author

bigbasec commented Feb 4, 2021

Should be all set now, I hope. Really need to learn more about Git, I'm terrible with this.

@kaikreuzer
Copy link
Member

Jenkins is still failing. Seems you didn't yet push the commit with the spotless fixes.

@bigbasec
Copy link
Contributor Author

bigbasec commented Feb 4, 2021

I only committed the one file, so I don't know. Ran the spotless:apply committed and that's all. I'll try again I guess.

@bigbasec
Copy link
Contributor Author

bigbasec commented Feb 4, 2021

Just ran it again and there would be no changes to the file in question, so I don't know what to do here. Sorry!

@kaikreuzer
Copy link
Member

There must be changes, otherwise Jenkins wouldn't fail. Does the Maven build succeed locally for you? Are you sure that you have pushed all your changes?

@bigbasec
Copy link
Contributor Author

bigbasec commented Feb 4, 2021

Nope, completely broken by the updates above to satisfy your recommendations. Give me a minute I'll fix all that.

@bigbasec
Copy link
Contributor Author

bigbasec commented Feb 4, 2021

5th time's a charm. Apologies, I'll get this sooner or later.

bigbasec and others added 7 commits February 4, 2021 19:30
Signed-off-by: Brian Homeyer <bhomeyer@gmail.com>
Signed-off-by: Brian Homeyer <bhomeyer@gmail.com>
Signed-off-by: Wouter Born <github@maindrain.net>
…2164)

Signed-off-by: Sami Salonen <ssalonen@gmail.com>
Signed-off-by: Brian Homeyer <bhomeyer@gmail.com>
Signed-off-by: Christian Bandowski <christian@myvm.de>
Signed-off-by: Brian Homeyer <bhomeyer@gmail.com>
Removing old sitemap reference

Signed-off-by: Brian Homeyer <bhomeyer@gmail.com>
@bigbasec
Copy link
Contributor Author

bigbasec commented Feb 5, 2021

Sorry, I tried. Again, I give up.

@bigbasec bigbasec closed this Feb 5, 2021
@bigbasec bigbasec deleted the add_provider_lookup_sitemaps branch February 5, 2021 00:52
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.

6 participants