Skip to content

fix: updated Fileservice.cc to handle mAdminKeys as value type#1276

Open
ParasSalonia wants to merge 4 commits intohiero-ledger:mainfrom
ParasSalonia:fix/file-service-admin-keys-handling
Open

fix: updated Fileservice.cc to handle mAdminKeys as value type#1276
ParasSalonia wants to merge 4 commits intohiero-ledger:mainfrom
ParasSalonia:fix/file-service-admin-keys-handling

Conversation

@ParasSalonia
Copy link
Copy Markdown
Contributor

@ParasSalonia ParasSalonia commented Mar 25, 2026

@ParasSalonia ParasSalonia marked this pull request as ready for review March 25, 2026 14:36
@ParasSalonia ParasSalonia requested review from a team as code owners March 25, 2026 14:36
@ParasSalonia ParasSalonia requested a review from gsstoykov March 25, 2026 14:36
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 25, 2026

Hey @ParasSalonia 👋 thanks for the PR!
I'm your friendly PR Helper Bot 🤖 and I'll be riding shotgun on this one, keeping track of your PR's status to help you get it approved and merged.

This comment updates automatically as you push changes -- think of it as your PR's live scoreboard!
Here's the latest:


PR Checks

DCO Sign-off -- All commits have valid sign-offs. Nice work!


GPG Signature -- All commits have verified GPG signatures. Locked and loaded!


Merge Conflicts -- No merge conflicts detected. Smooth sailing!


Issue Link -- Linked to #1272 (assigned to you).


🎉 All checks passed! Your PR is ready for review. Great job!

@github-actions github-actions bot added status: needs revision A pull request that requires changes before merge status: needs review The pull request is ready for maintainer review and removed status: needs revision A pull request that requires changes before merge labels Mar 25, 2026
Copy link
Copy Markdown
Contributor

@rwalworth rwalworth 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 quick turnaround on this @ParasSalonia! The fix is correct - mAdminKeys is a KeyList value type in FileInfo, so the pointer/optional semantics needed to go, and toBytes() is the right call here.

One small thing: the clang-format lint check is failing because the replacement block isn't indented to match the surrounding code. Once that's fixed, we'll be good to merge!

@rwalworth rwalworth added status: needs revision A pull request that requires changes before merge and removed status: needs review The pull request is ready for maintainer review labels Mar 26, 2026
@github-actions github-actions bot added status: needs review The pull request is ready for maintainer review and removed status: needs revision A pull request that requires changes before merge labels Mar 26, 2026
Copy link
Copy Markdown
Contributor

@rwalworth rwalworth 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 addressing the review feedback quickly, @ParasSalonia! The second commit fixes the brace positions, but the body of the if block still isn't indented to match the surrounding code. The clang-format check will continue to fail until this is resolved. Once the indentation is corrected the fix looks good and we can merge.

@rwalworth rwalworth added status: needs revision A pull request that requires changes before merge and removed status: needs review The pull request is ready for maintainer review labels Mar 26, 2026
@github-actions github-actions bot added status: needs review The pull request is ready for maintainer review and removed status: needs revision A pull request that requires changes before merge labels Mar 27, 2026
@ParasSalonia
Copy link
Copy Markdown
Contributor Author

Hi @rwalworth,

I have made the requested changes. Could you please review them and let me know if any further improvements are needed?

Copy link
Copy Markdown
Contributor

@rwalworth rwalworth left a comment

Choose a reason for hiding this comment

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

The formatting is still incorrect. Please run clang-format-17 -i src/tck/src/file/FileService.cc to fix.

@rwalworth rwalworth added status: needs revision A pull request that requires changes before merge and removed status: needs review The pull request is ready for maintainer review labels Mar 27, 2026
@ParasSalonia ParasSalonia removed their assignment Mar 28, 2026
@ParasSalonia ParasSalonia requested a review from rwalworth March 28, 2026 13:44
@codacy-production
Copy link
Copy Markdown

codacy-production bot commented Mar 31, 2026

Not up to standards ⛔

🔴 Issues 1 medium

Alerts:
⚠ 1 issue (≤ 0 issues of at least minor severity)

Results:
1 new issue

Category Results
Complexity 1 medium

View in Codacy

🟢 Metrics 0 complexity

Metric Results
Complexity 0

View in Codacy

TIP This summary will be updated as you push new changes. Give us feedback

@ParasSalonia ParasSalonia force-pushed the fix/file-service-admin-keys-handling branch from 54f913d to 5b68547 Compare March 31, 2026 15:17
@github-actions github-actions bot removed the status: needs revision A pull request that requires changes before merge label Mar 31, 2026
@ParasSalonia ParasSalonia requested a review from nathanklick April 3, 2026 20:45
@github-actions github-actions bot added status: needs review The pull request is ready for maintainer review and removed status: needs revision A pull request that requires changes before merge labels Apr 3, 2026
@ParasSalonia ParasSalonia marked this pull request as draft April 3, 2026 20:45
@ParasSalonia ParasSalonia force-pushed the fix/file-service-admin-keys-handling branch 5 times, most recently from 3e13f54 to 1e4ba3a Compare April 3, 2026 21:05
Signed-off-by: ParasSalonia <parassalonia22@gmail.com>
@ParasSalonia ParasSalonia force-pushed the fix/file-service-admin-keys-handling branch from 1e4ba3a to 34e71ec Compare April 3, 2026 21:26
@ParasSalonia ParasSalonia marked this pull request as ready for review April 3, 2026 21:29
@ParasSalonia
Copy link
Copy Markdown
Contributor Author

Thanks for the guidance! I rebased onto the latest main and re-ran clang-format from the repo root. I’ve pushed the updated changes — please let me know if everything looks correct now.

Copy link
Copy Markdown
Contributor

@rwalworth rwalworth left a comment

Choose a reason for hiding this comment

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

Hey @ParasSalonia, what version of clang-format are you running locally? It looks like you may be running a different version than what CI uses - the CI pins clang-format-17, and different versions produce different output for the same .clang-format config (the spaces-inside-braces behavior is one of those areas).

If you install and use clang-format-17 explicitly, it should match:

# Ubuntu/Debian
sudo apt install clang-format-17

# Then format and verify (from the repo root)
clang-format-17 -i src/tck/src/file/FileService.cc
clang-format-17 src/tck/src/file/FileService.cc | diff src/tck/src/file/FileService.cc -

No output from the diff means the file matches what CI expects. Once the lint check goes green, we can merge!

@rwalworth rwalworth added status: needs revision A pull request that requires changes before merge and removed status: needs review The pull request is ready for maintainer review labels Apr 4, 2026
@ParasSalonia ParasSalonia marked this pull request as draft April 4, 2026 06:15
@ParasSalonia ParasSalonia marked this pull request as ready for review April 4, 2026 06:24
@github-actions github-actions bot added status: needs review The pull request is ready for maintainer review and removed status: needs revision A pull request that requires changes before merge labels Apr 4, 2026
@rwalworth rwalworth added status: needs revision A pull request that requires changes before merge and removed status: needs review The pull request is ready for maintainer review labels Apr 4, 2026
Signed-off-by: ParasSalonia <parassalonia22@gmail.com>
@ParasSalonia ParasSalonia marked this pull request as draft April 4, 2026 14:40
@github-actions github-actions bot added status: needs review The pull request is ready for maintainer review and removed status: needs revision A pull request that requires changes before merge labels Apr 4, 2026
@ParasSalonia ParasSalonia marked this pull request as ready for review April 4, 2026 14:55
@ParasSalonia
Copy link
Copy Markdown
Contributor Author

Hi @rwalworth,

  • The latest changes include only the functional fix with correct formatting. The lint checks are now passing

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

Labels

status: needs review The pull request is ready for maintainer review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Beginner]: Build failure in TCK FileService.cc due to FileInfo mAdminKeys API change

2 participants