-
Notifications
You must be signed in to change notification settings - Fork 2.6k
feat: Remove debug permission #4067
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
Conversation
|
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
| RESOURCE_APPLICATION_DEBUG: new Permission('SYSTEM_RESOURCE_APPLICATION:READ+DEBUG'), | ||
| RESOURCE_APPLICATION_IMPORT: new Permission('SYSTEM_RESOURCE_APPLICATION:READ+IMPORT'), | ||
| RESOURCE_APPLICATION_EXPORT: new Permission('SYSTEM_RESOURCE_APPLICATION:READ+EXPORT'), | ||
| RESOURCE_APPLICATION_DELETE: new Permission('SYSTEM_RESOURCE_APPLICATION:READ+DELETE'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The provided TypeScript code is mostly clean but contains some stylistic adjustments that can be made to improve readability and maintainability. Here's a list of minor improvements:
-
Variable Name Consistency: Ensure consistent variable naming conventions, especially if there are related variables being used together.
-
Semicolon Use: Add semicolons after each declaration statement for better readability, although in this context it might not be necessary as long as line breaks naturally separate statements.
-
Spacing: Improve spacing between keywords, operators, and brackets where appropriate.
Here’s the revised version with these changes:
import { Permission, Role, Edition } from '@/utils/permission/type';
const PermissionConst = {
SHARED_TOOL_READ: new Permission('SYSTEM_TOOL:READ'),
SHARED_TOOL_CREATE: new Permission('SYSTEM_TOOL:READ+CREATE'),
SHARED_TOOL_IMPORT: new Permission('SYSTEM_TOOL:READ+IMPORT'),
SHARED_TOOL_EXPORT: new Permission('SYSTEM_TOOL:READ+EXPORT'),
- // SHARED_TOOL_DEBUG: new Permission('SYSTEM_TOOL:READ+DEBUG'), # This seems like an obsolete comment or commented out section
SHARED_MODEL_READ: new Permission('SYSTEM_MODEL:READ'),
SHARED_MODEL_CREATE: new Permission('SYSTEM_MODEL:READ+CREATE'),
SHARED_RESOURCE_TOOL_READ: new Permission('SYSTEM_RESOURCE_TOOL:READ'),
SHARED_RESOURCE_TOOL_CREATE: new Permission('SYSTEM_RESOURCE_TOOL:READ+CREATE'),
SHARED_RESOURCE_TOOL_EDIT: new Permission('SYSTEM_RESOURCE_TOOL:READ+EDIT'),
SHARED_RESOURCE_TOOL_DELETE: new Permission('SYSTEM_RESOURCE_TOOL:READ+DELETE'),
-// SHARED_RESOURCE_TOOL_DEBUG: new Permission('SYSTEM_RESOURCE_TOOL:READ+DEBUG'), # Also commented out or unused
SHARED_RESOURCE_TOOL_IMPORT: new Permission('SYSTEM_RESOURCE_TOOL:READ+IMPORT'),
SHARED_RESOURCE_TOOL_EXPORT: new Permission('SYSTEM_RESOURCE_TOOL:READ+EXPORT'),
RESOURCES_APP_READ: new Permission('SYSTEM_RESOURCE_APPLICATION:READ'),
RESOURCES_APP_EDIT: new Permission('SYSTEM_RESOURCE_APPLICATION:READ+EDIT'),
RESOURCES_APP_DELETE: new Permission('SYSTEM_RESOURCE_APPLICATION:READ+DELETE'),
};These changes make the code slightly more readable at first glance while keeping the logic intact. If you have specific areas within the PermissionConst object that need further attention (e.g., comments indicating obsolescence), please feel free to specify!
| PermissionConst.RESOURCE_APPLICATION_EDIT | ||
| ], | ||
| 'OR' | ||
| ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The provided line of code seems to be checking permissions for accessing resource application debugging information. The change from ResourceApplicationDebug to ResourceApplicationEdit suggests that the intended functionality is likely changing.
Possible Issues:
- Inconsistency: There might be documentation inconsistencies regarding which permission should be used for this action, leading to confusion about what users are supposed to have access to.
Optimization Suggestions:
- Consider adding comments explaining why this specific permission was changed from one role const to another.
- If there's a need for further clarification or additional checks, ensure those aspects are included in the codebase for future maintenance and understanding.
Overall, the line appears valid but may require some context-based justification or documentation updates.
| PermissionConst.RESOURCE_TOOL_EDIT, | ||
| ], | ||
| 'OR', | ||
| ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The proposed change seems to be addressing a typo in the permission requirement. Instead of PermissionConst.RESOURCE_TOOL_DEBUG, it should use PermissionConst.RESOURCE_TOOL_EDIT. This will allow users with EDIT permissions for resource tools to access certain system management features.
However, I would recommend adding error handling or logging mechanisms around this logic if there are other permissions or roles involved that might affect system operations. Additionally, you could consider whether these changes need to impact other parts of the application or require additional testing before deployment.
feat: Remove debug permission