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

chore!: remove swirlds-commons dependency from swirlds-config-impl #10386

Merged
merged 3 commits into from
Dec 11, 2023

Conversation

timo0
Copy link
Member

@timo0 timo0 commented Dec 8, 2023

Fix #9002

Signed-off-by: Timo Brandstätter <timo@swirldslabs.com>
@timo0 timo0 requested review from a team as code owners December 8, 2023 14:49
@timo0 timo0 requested a review from a team December 8, 2023 14:49
@timo0 timo0 requested review from a team and tinker-michaelj as code owners December 8, 2023 14:49
@timo0 timo0 added this to the v0.46 milestone Dec 8, 2023
@timo0 timo0 requested a review from hendrikebbers December 8, 2023 14:50
@timo0 timo0 self-assigned this Dec 8, 2023
Copy link

github-actions bot commented Dec 8, 2023

Node: HAPI Test (Token) Results

189 tests  ±0   189 ✔️ ±0   23m 29s ⏱️ + 4m 49s
  13 suites ±0       0 💤 ±0 
  13 files   ±0       0 ±0 

Results for commit eefdf33. ± Comparison against base commit d0e5885.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Dec 8, 2023

Node: HAPI Test (Crypto) Results

211 tests  ±0   201 ✔️ ±0   20m 30s ⏱️ + 3m 13s
  22 suites ±0     10 💤 ±0 
  22 files   ±0       0 ±0 

Results for commit eefdf33. ± Comparison against base commit d0e5885.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Dec 8, 2023

Node: Unit Test Results

    2 291 files   - 2      2 291 suites   - 2   45m 55s ⏱️ - 1m 48s
118 424 tests  - 4  118 390 ✔️  - 4  34 💤 ±0  0 ±0 
126 845 runs   - 4  126 811 ✔️  - 4  34 💤 ±0  0 ±0 

Results for commit eefdf33. ± Comparison against base commit d0e5885.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Dec 8, 2023

Node: HAPI Test (Time Consuming) Results

21 tests  ±0     9 ✔️ ±0   26m 4s ⏱️ + 1m 31s
  2 suites ±0   12 💤 ±0 
  2 files   ±0     0 ±0 

Results for commit eefdf33. ± Comparison against base commit d0e5885.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Dec 8, 2023

Node: HAPI Test (Misc) Results

420 tests  ±0   318 ✔️ +1   27m 16s ⏱️ + 3m 18s
  74 suites ±0   102 💤 ±0 
  74 files   ±0       0  - 1 

Results for commit eefdf33. ± Comparison against base commit d0e5885.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Dec 8, 2023

Node: E2E Test Results

    1 files  ±    0      1 suites  ±0   21m 49s ⏱️ + 21m 49s
311 tests +310  311 ✔️ +311  0 💤 ±0  0  - 1 
333 runs  +332  333 ✔️ +333  0 💤 ±0  0  - 1 

Results for commit eefdf33. ± Comparison against base commit d0e5885.

This pull request removes 1 and adds 311 tests. Note that renamed tests count towards both.
EndToEndTests ‑ initializationError
EndToEndTests ‑ ADDRESS_BOOK_CONTROLCanUpdateADDRESS_BOOK
EndToEndTests ‑ ADDRESS_BOOK_CONTROLCanUpdateNODE_DETAILS
EndToEndTests ‑ AccountsGetPayerRecordsIfSoConfigured
EndToEndTests ‑ Acct57CanMakeSmallChanges
EndToEndTests ‑ Acct57CantMakeLargeChanges
EndToEndTests ‑ AddingSignaturesToExecutedTxFails
EndToEndTests ‑ AddingSignaturesToExecutedTxFailsWithLongTermEnabled
EndToEndTests ‑ AddingSignaturesToNonExistingTxFails
EndToEndTests ‑ AddingSignaturesToNonExistingTxFailsWithLongTermEnabled
EndToEndTests ‑ AddressAliasIdFuzzing
…

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Dec 8, 2023

Node: Integration Test Results

279 tests  ±0   279 ✔️ ±0   28m 34s ⏱️ +9s
    5 suites ±0       0 💤 ±0 
    5 files   ±0       0 ±0 

Results for commit eefdf33. ± Comparison against base commit d0e5885.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Dec 8, 2023

Node: HAPI Test (Smart Contract) Results

397 tests  ±0   356 ✔️ ±0   45m 53s ⏱️ +6s
  55 suites ±0     41 💤 ±0 
  55 files   ±0       0 ±0 

Results for commit eefdf33. ± Comparison against base commit d0e5885.

♻️ This comment has been updated with latest results.

cody-littley
cody-littley previously approved these changes Dec 8, 2023
# Conflicts:
#	platform-sdk/swirlds-common/src/test/java/com/swirlds/common/metrics/platform/prometheus/PrometheusEndpointTest.java
#	platform-sdk/swirlds-platform-core/src/main/java/com/swirlds/platform/event/preconsensus/PreconsensusEventFileManager.java
#	platform-sdk/swirlds-platform-core/src/main/java/com/swirlds/platform/metrics/SyncMetrics.java
Copy link

codecov bot commented Dec 8, 2023

Codecov Report

Attention: 11 lines in your changes are missing coverage. Please review.

Comparison is base (d0e5885) 62.97% compared to head (eefdf33) 62.97%.
Report is 10 commits behind head on develop.

Files Patch % Lines
...rlds/config/impl/converters/DurationConverter.java 75.00% 8 Missing and 3 partials ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##             develop   #10386      +/-   ##
=============================================
- Coverage      62.97%   62.97%   -0.01%     
- Complexity     30794    30795       +1     
=============================================
  Files           3356     3354       -2     
  Lines         135216   135208       -8     
  Branches       14076    14076              
=============================================
- Hits           85152    85144       -8     
+ Misses         46728    46727       -1     
- Partials        3336     3337       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@timo0 timo0 requested a review from cody-littley December 9, 2023 00:19
Copy link
Contributor

@povolev15 povolev15 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@Neeharika-Sompalli Neeharika-Sompalli left a comment

Choose a reason for hiding this comment

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

Based on files owned by hedera-base LGTM

Copy link
Member

@david-bakin-sl david-bakin-sl left a comment

Choose a reason for hiding this comment

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

Looking only at hedera-smart-contract-service LGTM. (There was nothing contract related in hedera-mono-service.

Suggest changing PR title and/or PR comment to make it clear, here in this PR, that the common you're talking about is swirlds-common and not, say, Apache common. I mean, it's clear from the code, but if you're looking at, say, release notes, it might be nicer to have it there.

@timo0 timo0 changed the title chore!: remove commons dependency from config-impl chore!: remove swirlds-common dependency from swirlds-config-impl Dec 11, 2023
@timo0 timo0 changed the title chore!: remove swirlds-common dependency from swirlds-config-impl chore!: remove swirlds-common dependency from swirlds-config-impl Dec 11, 2023
@timo0 timo0 changed the title chore!: remove swirlds-common dependency from swirlds-config-impl chore!: remove swirlds-common dependency from swirlds-config-impl Dec 11, 2023
@timo0 timo0 changed the title chore!: remove swirlds-common dependency from swirlds-config-impl chore!: remove swirlds-commons dependency from swirlds-config-impl Dec 11, 2023
@mxtartaglia-sl mxtartaglia-sl merged commit 4123932 into develop Dec 11, 2023
28 checks passed
@mxtartaglia-sl mxtartaglia-sl deleted the 09002-d-remove-commons-from-config-impl branch December 11, 2023 20:24
nathanklick pushed a commit that referenced this pull request Dec 12, 2023
…#10386)

Signed-off-by: Timo Brandstätter <timo@swirldslabs.com>
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.

config-impl should not depend on swirlds-common
9 participants