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

feat(logs): remove logger abstraction #348

Merged
merged 1 commit into from
Feb 14, 2024
Merged

feat(logs): remove logger abstraction #348

merged 1 commit into from
Feb 14, 2024

Conversation

mwangggg
Copy link
Member

fixes: #52

@mwangggg mwangggg added chore Refactor, rename, cleanup, etc. safe-to-test feat New feature or request and removed chore Refactor, rename, cleanup, etc. labels Feb 12, 2024
@mwangggg mwangggg changed the title chore(logs): remove logger abstraction feat(logs): remove logger abstraction Feb 12, 2024
@mwangggg
Copy link
Member Author

/build_test

@mwangggg
Copy link
Member Author

oh I didn't notice #322 is there any overlap?

@github-actions GitHub Actions
Copy link

Cryostat Test: At least one test failed ❌
https://github.com/cryostatio/cryostat-core/actions/runs/7874901459

@github-actions GitHub Actions
Copy link

Image does not exist! Please wait for the initial build and push to complete before using /build_test command.

@mwangggg
Copy link
Member Author

/build_test

@github-actions GitHub Actions
Copy link

Cryostat Test: At least one test failed ❌
https://github.com/cryostatio/cryostat-core/actions/runs/7875304884

@andrewazores
Copy link
Member

I think #52 probably also solves or is a replacement for #39, so in that case this PR also replaces #322.

@andrewazores
Copy link
Member

Test failure looks like just a sporadic flakey test.

@mwangggg
Copy link
Member Author

/build_test

@andrewazores
Copy link
Member

https://github.com/cryostatio/cryostat-core/actions/runs/7877293531

@github-actions GitHub Actions
Copy link

Cryostat Test: At least one test failed ❌
https://github.com/cryostatio/cryostat-core/actions/runs/7877293531

@andrewazores
Copy link
Member

Different sporadic test failures, fun...

@andrewazores
Copy link
Member

/build_test

@andrewazores
Copy link
Member

https://github.com/cryostatio/cryostat-core/actions/runs/7877448301

@github-actions GitHub Actions
Copy link

Cryostat Test: At least one test failed ❌
https://github.com/cryostatio/cryostat-core/actions/runs/7877448301

@mwangggg
Copy link
Member Author

:o so many failures

@andrewazores
Copy link
Member

Yea, this is getting ridiculous. Hitting every sporadic failure I've seen for the 2.x codebase in a row.

@andrewazores
Copy link
Member

/build_test

@andrewazores
Copy link
Member

https://github.com/cryostatio/cryostat-core/actions/runs/7877597207

@github-actions GitHub Actions
Copy link

Cryostat Test: All tests pass ✅
https://github.com/cryostatio/cryostat-core/actions/runs/7877597207

andrewazores
andrewazores requested changes Feb 13, 2024
View reviewed changes
Copy link
Member

@andrewazores andrewazores left a comment

Choose a reason for hiding this comment

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

Actually, just a couple of minor nitpicks before merge.

src/test/java/io/cryostat/core/net/discovery/JvmDiscoveryClientTest.java Show resolved Hide resolved
src/test/java/io/cryostat/core/reports/InterruptibleReportGeneratorTest.java Outdated Show resolved Hide resolved
src/main/java/io/cryostat/core/net/discovery/JvmDiscoveryClient.java Outdated Show resolved Hide resolved
@mwangggg
Copy link
Member Author

/build_test

@github-actions GitHub Actions
Copy link

Cryostat Test: All tests pass ✅
https://github.com/cryostatio/cryostat-core/actions/runs/7905233960

@github-actions GitHub Actions
Copy link

ARCH IMAGE
amd64 ghcr.io/cryostatio/cryostat-core:pr-348-7a44ca64fc17575daa32ec13450b4a04f72cd929-linux-amd64
arm64 ghcr.io/cryostatio/cryostat-core:pr-348-7a44ca64fc17575daa32ec13450b4a04f72cd929-linux-arm64
cryostat3-java17_amd64 ghcr.io/cryostatio/cryostat-core:pr-348-7a44ca64fc17575daa32ec13450b4a04f72cd929-linux-amd64-java17
cryostat3-java17_arm64 ghcr.io/cryostatio/cryostat-core:pr-348-7a44ca64fc17575daa32ec13450b4a04f72cd929-linux-arm64-java17

To run smoketest:

# amd64
CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-core:pr-348-7a44ca64fc17575daa32ec13450b4a04f72cd929-linux-amd64 sh smoketest.sh

# or arm64
CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-core:pr-348-7a44ca64fc17575daa32ec13450b4a04f72cd929-linux-arm64 sh smoketest.sh

# or cryostat3-java17-amd64
CRYOSTAT3_java17amd64_IMAGE=ghcr.io/cryostatio/cryostat-core:pr-348-7a44ca64fc17575daa32ec13450b4a04f72cd929-linux-amd64-java17 sh smoketest.sh

# or cryostat3-java17-arm64
CRYOSTAT3_java17arm64_IMAGE=ghcr.io/cryostatio/cryostat-core:pr-348-7a44ca64fc17575daa32ec13450b4a04f72cd929-linux-arm64-java17 sh smoketest.sh

@mwangggg
Copy link
Member Author

/build_test

@github-actions GitHub Actions
Copy link

Cryostat Test: All tests pass ✅
https://github.com/cryostatio/cryostat-core/actions/runs/7907703338

@andrewazores andrewazores added the breaking change This change (potentially) breaks API compatibility and requires corresponding changes elsewhere label Feb 14, 2024
@andrewazores andrewazores merged commit 684a061 into cryostatio:main Feb 14, 2024
19 checks passed
@github-actions GitHub Actions
Copy link

ARCH IMAGE
amd64 ghcr.io/cryostatio/cryostat-core:pr-348-7a44ca64fc17575daa32ec13450b4a04f72cd929-linux-amd64
arm64 ghcr.io/cryostatio/cryostat-core:pr-348-7a44ca64fc17575daa32ec13450b4a04f72cd929-linux-arm64
cryostat3-java17_amd64 ghcr.io/cryostatio/cryostat-core:pr-348-7a44ca64fc17575daa32ec13450b4a04f72cd929-linux-amd64-java17
cryostat3-java17_arm64 ghcr.io/cryostatio/cryostat-core:pr-348-7a44ca64fc17575daa32ec13450b4a04f72cd929-linux-arm64-java17

To run smoketest:

# amd64
CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-core:pr-348-7a44ca64fc17575daa32ec13450b4a04f72cd929-linux-amd64 sh smoketest.sh

# or arm64
CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-core:pr-348-7a44ca64fc17575daa32ec13450b4a04f72cd929-linux-arm64 sh smoketest.sh

# or cryostat3-java17-amd64
CRYOSTAT3_java17amd64_IMAGE=ghcr.io/cryostatio/cryostat-core:pr-348-7a44ca64fc17575daa32ec13450b4a04f72cd929-linux-amd64-java17 sh smoketest.sh

# or cryostat3-java17-arm64
CRYOSTAT3_java17arm64_IMAGE=ghcr.io/cryostatio/cryostat-core:pr-348-7a44ca64fc17575daa32ec13450b4a04f72cd929-linux-arm64-java17 sh smoketest.sh

@andrewazores andrewazores mentioned this pull request Feb 22, 2024
Merged
@mwangggg mwangggg deleted the 52-remove-logger branch March 12, 2024 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewazores andrewazores andrewazores approved these changes

@aali309 aali309 Awaiting requested review from aali309

Assignees
No one assigned
Labels
breaking change This change (potentially) breaks API compatibility and requires corresponding changes elsewhere feat New feature or request safe-to-test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove Logger abstraction
2 participants
@mwangggg @andrewazores