Skip to content

Improve logs on IpmitoolOutOfBandManagementDriver#4702

Merged
DaanHoogland merged 1 commit into
apache:masterfrom
GutoVeronezi:improve-logs-on-ipmitooloutofbandmanagementdriver
Apr 21, 2021
Merged

Improve logs on IpmitoolOutOfBandManagementDriver#4702
DaanHoogland merged 1 commit into
apache:masterfrom
GutoVeronezi:improve-logs-on-ipmitooloutofbandmanagementdriver

Conversation

@GutoVeronezi

Copy link
Copy Markdown
Contributor

Description

This PR intends to improve logging in the class IpmitoolOutOfBandManagementDriver to facilitate troubleshooting.

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functioanlity)
  • Cleanup (Code refactoring and cleanup, that may add test cases)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

How Has This Been Tested?

It has been tested locally on a test lab.

@DaanHoogland DaanHoogland left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

code looks good

import java.util.List;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import org.apache.commons.lang3.StringUtils;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

please use our own (cloudstack) Stringutils as proxy, even when calling the apache.commons

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There is no consensus on that. I would prefer this way, using the library directly. We do not get benefits on a facade like that.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

yes we do, and we are badly hampered in most cases where we don't (gson/logging mostly)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So, lets add a facade to Spring as well, and to any other utils library that we use :)

I still see no benefits on creating such facades.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

that is a lazy exaggeration, we have a facade for string utils and we are hampered by several 3rd party libraries for which we don't. If there are resources for making such a thing for spring, great. Let's do it.

BTW, in this case I didn't 👎 on this issue, but on other counts I might as well. duckduck "using 3rd party library facades", will give you some wisdom on this.

out

@GabrielBrascher GabrielBrascher left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Code LGTM.

@GutoVeronezi GutoVeronezi mentioned this pull request Feb 22, 2021
7 tasks
@yadvr

yadvr commented Mar 6, 2021

Copy link
Copy Markdown
Member

@blueorangutan package

@blueorangutan

Copy link
Copy Markdown

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@blueorangutan

Copy link
Copy Markdown

Packaging result: ✖centos7 ✖centos8 ✖debian. JID-2881

@GutoVeronezi

Copy link
Copy Markdown
Contributor Author

@blueorangutan package

@blueorangutan

Copy link
Copy Markdown

@GutoVeronezi a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@blueorangutan

Copy link
Copy Markdown

Packaging result: ✔️ centos7 ✔️ centos8 ✔️ debian. SL-JID 396

@yadvr

yadvr commented Apr 12, 2021

Copy link
Copy Markdown
Member

Re-kicking travis

@yadvr yadvr closed this Apr 12, 2021
@yadvr yadvr reopened this Apr 12, 2021
@GutoVeronezi

Copy link
Copy Markdown
Contributor Author

@rhtyd @GabrielBrascher @DaanHoogland is there anything else to do?

@DaanHoogland DaanHoogland merged commit 3c8a504 into apache:master Apr 21, 2021
@DaanHoogland

Copy link
Copy Markdown
Contributor

that's a no

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants