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

[weatherunderground] Log URL with masked API key #7756

Merged
merged 2 commits into from
May 25, 2020

Conversation

lolodomo
Copy link
Contributor

Relative to #3990

Signed-off-by: Laurent Garnier lg.hc@free.fr

Relative to openhab#3990

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
@TravisBuddy
Copy link

Travis tests were successful

Hey @lolodomo,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

@@ -553,7 +551,9 @@ private boolean updateWeatherData(Set<String> features) {
}

urlStr = urlStr.replace("%QUERY%", StringUtils.trimToEmpty(config.location));
logger.debug("URL = {}", urlStr);
logger.debug("URL = {}", urlStr.replace("%APIKEY%", "***"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
logger.debug("URL = {}", urlStr.replace("%APIKEY%", "***"));
if(logger.isDebugEnabled()){
logger.debug("URL = {}", urlStr.replace("%APIKEY%", "***"));
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What's the interest to add this test?

Copy link
Contributor

Choose a reason for hiding this comment

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

Since users may not be interested in debug logging, and as such would be filtered out, you should try to minimize wasting computational resources on creating values to be logged.
In this case String.replace involves a search and replace, the result of which might not even be used. So in order to prevent unnecessary calls to this operation we just check if that logging level is even enabled in the first place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, done.

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
@TravisBuddy
Copy link

Travis tests were successful

Hey @lolodomo,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

@cpmeister cpmeister merged commit e787948 into openhab:2.5.x May 25, 2020
@cpmeister cpmeister added this to the 2.5.6 milestone May 25, 2020
bodiroga pushed a commit to bodiroga/openhab-addons that referenced this pull request May 26, 2020
* [weatherunderground] Log URL with masked API key

Relative to openhab#3990
* Add isDebugEnabled

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
LoungeFlyZ pushed a commit to LoungeFlyZ/openhab2-addons that referenced this pull request Jun 8, 2020
* [weatherunderground] Log URL with masked API key

Relative to openhab#3990
* Add isDebugEnabled

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
J-N-K pushed a commit to J-N-K/openhab-addons that referenced this pull request Jul 14, 2020
* [weatherunderground] Log URL with masked API key

Relative to openhab#3990
* Add isDebugEnabled

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
CSchlipp pushed a commit to CSchlipp/openhab-addons that referenced this pull request Jul 26, 2020
* [weatherunderground] Log URL with masked API key

Relative to openhab#3990
* Add isDebugEnabled

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
Signed-off-by: CSchlipp <christian@schlipp.de>
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
* [weatherunderground] Log URL with masked API key

Relative to openhab#3990
* Add isDebugEnabled

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
* [weatherunderground] Log URL with masked API key

Relative to openhab#3990
* Add isDebugEnabled

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
* [weatherunderground] Log URL with masked API key

Relative to openhab#3990
* Add isDebugEnabled

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
* [weatherunderground] Log URL with masked API key

Relative to openhab#3990
* Add isDebugEnabled

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
DaanMeijer pushed a commit to DaanMeijer/openhab-addons that referenced this pull request Sep 1, 2020
* [weatherunderground] Log URL with masked API key

Relative to openhab#3990
* Add isDebugEnabled

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
Signed-off-by: Daan Meijer <daan@studioseptember.nl>
markus7017 pushed a commit to markus7017/openhab-addons that referenced this pull request Sep 19, 2020
* [weatherunderground] Log URL with masked API key

Relative to openhab#3990
* Add isDebugEnabled

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
@lolodomo lolodomo deleted the wu_hideapikey branch October 10, 2020 09:58
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.

3 participants