Skip to content

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

Open
ParasSalonia wants to merge 7 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 7 commits intohiero-ledger:mainfrom
ParasSalonia:fix/file-service-admin-keys-handling

Conversation

@ParasSalonia
Copy link
Copy Markdown
Contributor

@ParasSalonia ParasSalonia commented Mar 25, 2026

Signed-off-by: ParasSalonia <parassalonia22@gmail.com>
@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
Signed-off-by: Paras Salonia <parassalonia22@gmail.com>
@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
Signed-off-by: Paras Salonia <parassalonia22@gmail.com>
@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

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 complexity . 0 duplication

Metric Results
Complexity 0
Duplication 0

View in Codacy

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

Signed-off-by: ParasSalonia <parassalonia22@gmail.com>
@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 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 31, 2026
@ParasSalonia
Copy link
Copy Markdown
Contributor Author

Hi @rwalworth, I’ve fixed the GPG signing issue and updated the commits. Please let me know if everything looks good 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.

The clang-format check is failing again, but this time for a different reason. The spacing changes you made in the return blocks throughout the file (e.g., { "status", ... } with spaces inside the braces) are being rejected by the project's clang-format config. Those changes weren't needed - the original formatting in those blocks was already correct.

The root cause is likely that you ran clang-format-17 -i from a subdirectory rather than the repository root. When run from outside the root, clang-format can't find the project's .clang-format file and falls back to a different style.

Please run:

clang-format-17 -i src/tck/src/file/FileService.cc

from the repository root (the directory containing .clang-format). This should revert the spacing in those return blocks back to the original style while keeping the actual fix in getFileInfo. Once that's done we're good to go!


return {
{"status",
{ "status",
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.

issue (blocking): The space inside the braces here ({ "status",) is causing the clang-format check to fail. This change wasn't needed - the original {"status", was already correctly formatted per the project's .clang-format config.

The same problem exists in the other return blocks throughout the file (lines 120, 142, 172, 259).

Please run clang-format-17 -i src/tck/src/file/FileService.cc from the repository root (the directory containing .clang-format). If run from a subdirectory, clang-format won't find the project config and will apply a different style. Running from the root will restore the original formatting in these blocks while keeping the actual fix in getFileInfo.

@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 31, 2026
Signed-off-by: ParasSalonia <parassalonia22@gmail.com>
@ParasSalonia ParasSalonia force-pushed the fix/file-service-admin-keys-handling branch from 10378e0 to 90a86c6 Compare March 31, 2026 20:30
@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 31, 2026
@ParasSalonia
Copy link
Copy Markdown
Contributor Author

Thanks! I’ve re-run clang-format from the repo root and updated the formatting accordingly. I’ve also fixed the GPG signing and pushed the changes. Please let me know if anything else needs adjustment.

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.

Sorry @ParasSalonia, the lint check is still failing, and looking at the diff the spaces inside the braces ({ "status", ... }) are still there. I verified locally: when clang-format runs on the current main file from the repo root it makes zero changes - so the original {"status", ...} format (without spaces) is correct, and those spacing changes shouldn't be in the PR at all.

The functional change in getFileInfo is the only thing that should differ from main. Here are some things to try to narrow down why clang-format is producing different output on your end:

1. Verify clang-format is actually reading the project config:

clang-format --dump-config src/tck/src/file/FileService.cc | grep -E "BasedOnStyle|Cpp11BracedListStyle"

The first line should show BasedOnStyle: "". If it shows Google, LLVM, or anything else, clang-format isn't finding .clang-format.

2. Check for a stray .clang-format in a subdirectory (would shadow the root one):

find . -name ".clang-format" 2>/dev/null

Should print exactly one result at the repo root.

3. Check for a home-directory config:

ls ~/.clang-format 2>/dev/null && echo "Found — this may be overriding the project config"

4. Verify your version - must be 17.x:

clang-format --version

Older versions can produce different results with the same config.

5. Disable editor "Format on Save" - if VS Code or another editor is formatting the file on save, it may apply a different style and silently undo a correct clang-format run before you commit.

6. Confirm the fix directly:

clang-format src/tck/src/file/FileService.cc | diff src/tck/src/file/FileService.cc -

No output = file is correctly formatted. Once that's clean, we're 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 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status: needs revision A pull request that requires changes before merge

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