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.

Sign up for GitHub

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

Jump to bottom

Invalid logger breaks current application flow #753

Merged
merged 1 commit into from
May 10, 2021
Merged

Invalid logger breaks current application flow #753

merged 1 commit into from
May 10, 2021

Conversation

pjgg
Copy link
Contributor

@pjgg pjgg commented Apr 27, 2021 •
edited
Loading

Instead of throw an IllegalStateException exception we could log an error msg and swap to std out.

Related issue: quarkusio/quarkus#15953

@pjgg pjgg force-pushed the fix/invalidLogger branch from 9b02c40 to 6a83496 Compare April 27, 2021 14:26
iocanel
iocanel requested changes May 6, 2021
View reviewed changes
Copy link
Member

@iocanel iocanel left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request.
I have a minor aesthetic change request. Other than that it looks good.

Invalid logger breaks current application flow
6c616bd 
if a invalid ansi logger configuration is given, swap to default sdt out
instead of throw an `IllegalStateException` exception
@pjgg pjgg force-pushed the fix/invalidLogger branch from 6a83496 to 6c616bd Compare May 7, 2021 12:11
@sonarqubecloud
Copy link

sonarqubecloud bot commented May 7, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@pjgg pjgg requested a review from iocanel May 7, 2021 13:08
iocanel
iocanel approved these changes May 10, 2021
View reviewed changes
Copy link
Member

@iocanel iocanel left a comment

Choose a reason for hiding this comment

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

LGTM

@iocanel iocanel merged commit 117a0bd into dekorateio:master May 10, 2021
@geoand
Copy link
Collaborator

geoand commented May 10, 2021

@iocanel can we get a release of this that we then incorporate in Quarkus?

@iocanel
Copy link
Member

iocanel commented May 10, 2021 via email

@geoand
Copy link
Collaborator

geoand commented May 10, 2021

Thanks!

@iocanel
Copy link
Member

iocanel commented May 11, 2021

@pjgg: I have some just a little late questions:

  • Does this pull request solve the issue with quarkus (Have you verified the fix)?
  • Do you think that we should also bump jansi to a later version ?
  • Why do you use new FileOutputStream(FileDescriptor.out) vs System.out?

@geoand geoand mentioned this pull request May 12, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iocanel iocanel iocanel approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pjgg @geoand @iocanel