Skip to content

Conversation

@shirady
Copy link
Contributor

@shirady shirady commented Nov 3, 2025

Describe the Problem

While I read the code, I saw minor code lines that can easily be fixed.

Explain the Changes

  1. In iam_rest remove the extension of .js file in the require line.
  2. In iam_utils replace the catch clause as it was accidentally copied from the code lines above.
  3. In iam_delete_user fix the action name of the argument.
  4. In accountspace_fs fix the error logs printed line.

Issues:

  1. none

Testing Instructions:

  1. none
  • Doc added/updated
  • Tests added

Summary by CodeRabbit

  • Bug Fixes

    • Fixed validation for user deletion so the correct delete validation is applied.
    • Improved error translation for user deletion to surface the original RPC error.
  • Improvements

    • Enhanced quota error messages for access key limits with more contextual details.

@coderabbitai
Copy link

coderabbitai bot commented Nov 3, 2025

Walkthrough

Removed explicit .js extensions from IAM requires, simplified delete-user error handling to translate RPC errors directly, corrected iam_delete_user validation action to use DELETE_USER, and improved access-key quota logging with account ID and counts.

Changes

Cohort / File(s) Summary
IAM imports & utils
src/endpoint/iam/iam_rest.js, src/endpoint/iam/iam_utils.js
Removed explicit .js extensions from require calls in iam_rest.js; in validate_delete_user, catch block now calls translate_rpc_error(err) instead of re-running parameter validation.
IAM delete operation
src/endpoint/iam/ops/iam_delete_user.js
Validation now uses IAM_ACTIONS.DELETE_USER instead of IAM_ACTIONS.UPDATE_USER; rest of flow unchanged.
Account space logging
src/sdk/accountspace_fs.js
Enhanced error logging in _check_number_of_access_key_array to include requested_account._id, current access_keys.length, and MAX_NUMBER_OF_ACCESS_KEYS when throwing IamError.LimitExceeded.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

  • Review iam_utils.js change to ensure no missing validation paths when delegating to translate_rpc_error.
  • Confirm IAM_ACTIONS.DELETE_USER is the intended constant in iam_delete_user.js.

Suggested reviewers

  • aayushchouhan09
  • naveenpaul1
  • liranmauda

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Title check ❓ Inconclusive The pull request title "IAM | Minor Fixes" is vague and generic. While it correctly identifies the IAM domain, it uses the non-descriptive term "Minor Fixes" without conveying specific information about what was actually fixed. The description reveals four distinct fixes: removing .js extensions from requires, correcting error handling logic, fixing an action name parameter, and improving error logging. The title does not reflect the nature or scope of these changes. Consider revising the title to be more specific about the primary fix or the scope of changes. For example: "IAM | Fix action name and error handling" or "IAM | Multiple fixes to validation, requires, and logging" would better communicate the changeset's intent to reviewers scanning history.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 844bec6 and 5c3abfc.

📒 Files selected for processing (4)
  • src/endpoint/iam/iam_rest.js (1 hunks)
  • src/endpoint/iam/iam_utils.js (1 hunks)
  • src/endpoint/iam/ops/iam_delete_user.js (1 hunks)
  • src/sdk/accountspace_fs.js (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • src/sdk/accountspace_fs.js
  • src/endpoint/iam/iam_utils.js
  • src/endpoint/iam/ops/iam_delete_user.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: Build Noobaa Image
  • GitHub Check: run-package-lock-validation
  • GitHub Check: run-jest-unit-tests
🔇 Additional comments (1)
src/endpoint/iam/iam_rest.js (1)

92-119: No action required—code is already consistent.

Lines 75-90 and 92-119 both omit .js extensions uniformly. The original concern about inconsistency between these sections is unfounded; the code already follows a consistent pattern throughout.

Likely an incorrect or invalid review comment.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@shirady shirady self-assigned this Nov 3, 2025
Signed-off-by: shirady <[email protected]>
@shirady shirady merged commit a566cda into noobaa:master Nov 3, 2025
18 checks passed
@shirady shirady deleted the iam-minor-fixes branch November 3, 2025 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants