chore:[Devops-299]Asset governance flag #2265
Conversation
WalkthroughThe recent changes revolve around introducing an asset governance feature to PacBot. A flag ( Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WebApp
participant API
participant BuildPacBot
participant DB
User ->> WebApp: Access PacBot UI
WebApp ->> API: Request Asset Governance Status
API ->> BuildPacBot: Check Asset Governance Flag
BuildPacBot ->> DB: Retrieve Asset Governance Flag
DB -->> BuildPacBot: Return Flag Status
BuildPacBot -->> API: Asset Governance Flag Status
API -->> WebApp: Asset Governance Flag Status
WebApp -->> User: Display UI Based on Flag
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (8)
installer/resources/pacbot_app/build_ui_and_api.py (3)
Line range hint
17-17: Consider using Python's newer string formatting methods.- raise Exception("Pacbot CodeBase, %s, is Not Found!" % pom_file) + raise Exception(f"Pacbot CodeBase, {pom_file}, is Not Found!")
Line range hint
59-59: Improve exception handling by specifying the cause of the exception.- raise Exception("Not able to create directory to store API Jars and UI code") + raise Exception("Not able to create directory to store API Jars and UI code") from OE
Line range hint
67-67: Avoid redefining methods that do the same thing; consider removing one of the duplicatepre_generate_terraformmethods.- def pre_generate_terraform(self): - # To support latest terraform version - PyTerraform.change_tf_extension_to_tf_json()installer/files/scripts/build_pacbot.py (4)
Line range hint
9-9: Remove unnecessary inheritance fromobjectin Python 3.- class Buildpacbot(object): + class Buildpacbot:
Line range hint
77-77: Rename unused loop variables to_.- for (dirpath, dirnames, file_names) in os.walk(local_folder_path): + for (_dirpath, _dirnames, file_names) in os.walk(local_folder, path):Also applies to: 77-77
Line range hint
81-81: Use Python's newer string formatting methods for better readability and performance.- "Email template files are not found in %s" % str(local_folder_path) + f"Email template files are not found in {str(local_folder_path)}"Also applies to: 89-89, 113-113, 115-115, 138-138, 157-157, 159-159, 172-172, 174-174, 193-193, 257-257, 259-259, 265-265
Line range hint
112-115: Combine similarifbranches to simplify the code.- if "AD_AUTHENTICATION: false" in line: - lines[idx] = lines[idx].replace("AD_AUTHENTICATION: false", "AD_AUTHENTICATION: true") - if "qualysEnabled: false" in line: - lines[idx] = lines[idx].replace("qualysEnabled: false", f"qualysEnabled: {self.enable_vulnerability_feature}") + if "AD_AUTHENTICATION: false" in line or "qualysEnabled: false" in line: + lines[idx] = lines[idx].replace("AD_AUTHENTICATION: false", "AD_AUTHENTICATION: true").replace("qualysEnabled: false", f"qualysEnabled: {self.enable_vulnerability_feature}")installer/resources/pacbot_app/import_db.py (1)
Line range hint
3-3: Remove unused imports to clean up the code.- from core.terraform.utils import get_terraform_provider_file - from resources.pacbot_app.cloudwatch_log_groups.UiCloudWatchLogGroup import UiCloudWatchLogGroup - from resources.pacbot_app.cloudwatch_log_groups.ApiCloudWatchLogGroup import ApiCloudWatchLogGroup - from resources.pacbot_app.ecr.APIEcrRepository import APIEcrRepository - from resources.pacbot_app.ecr.UIEcrRepository import UIEcrRepository - from resources.pacbot_app.cloudwatch_log_groups import UiCloudWatchLogGroup, ApiCloudWatchLogGroup - from resources.data.aws_info import AwsRegion - from resources.iam.ecs_role.ECSRole import ECSRole - from resources.notification.function.SendNotificationFunction import SendNotificationFunctionAlso applies to: 8-8, 8-8, 9-9, 9-9, 10-10, 12-12, 13-13, 23-23
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- installer/files/scripts/build_pacbot.py (3 hunks)
- installer/resources/pacbot_app/build_ui_and_api.py (1 hunks)
- installer/resources/pacbot_app/files/DB.sql (2 hunks)
- installer/resources/pacbot_app/import_db.py (1 hunks)
- installer/settings/default.local.py (1 hunks)
- webapp/src/config/configurations.ts (1 hunks)
Files skipped from review due to trivial changes (2)
- installer/settings/default.local.py
- webapp/src/config/configurations.ts
Additional context used
Ruff
installer/resources/pacbot_app/build_ui_and_api.py
17-17: Use format specifiers instead of percent format (UP031)
Replace with format specifiers
59-59: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling (B904)
67-67: Redefinition of unused
pre_generate_terraformfrom line 14 (F811)installer/files/scripts/build_pacbot.py
9-9: Class
Buildpacbotinherits fromobject(UP004)Remove
objectinheritance
77-77: Loop control variable
dirpathnot used within loop body (B007)Rename unused
dirpathto_dirpath
77-77: Loop control variable
dirnamesnot used within loop body (B007)Rename unused
dirnamesto_dirnames
81-81: Use format specifiers instead of percent format (UP031)
Replace with format specifiers
89-89: Use format specifiers instead of percent format (UP031)
Replace with format specifiers
112-115: Combine
ifbranches using logicaloroperator (SIM114)Combine
ifbranches
113-113: Use format specifiers instead of percent format (UP031)
Replace with format specifiers
115-115: Use format specifiers instead of percent format (UP031)
Replace with format specifiers
138-138: Use format specifiers instead of percent format (UP031)
Replace with format specifiers
157-157: Use format specifiers instead of percent format (UP031)
Replace with format specifiers
159-159: Use format specifiers instead of percent format (UP031)
Replace with format specifiers
172-172: Use format specifiers instead of percent format (UP031)
Replace with format specifiers
174-174: Use format specifiers instead of percent format (UP031)
Replace with format specifiers
181-181: Unnecessary open mode parameters (UP015)
Remove open mode parameters
193-193: Use format specifiers instead of percent format (UP031)
Replace with format specifiers
208-209: Use a single
ifstatement instead of nestedifstatements (SIM102)Combine
ifstatements usingand
257-257: Use format specifiers instead of percent format (UP031)
Replace with format specifiers
259-259: Use format specifiers instead of percent format (UP031)
Replace with format specifiers
265-265: Use format specifiers instead of percent format (UP031)
Replace with format specifiers
installer/resources/pacbot_app/import_db.py
3-3:
core.terraform.utils.get_terraform_provider_fileimported but unused (F401)Remove unused import:
core.terraform.utils.get_terraform_provider_file
8-8:
resources.pacbot_app.cloudwatch_log_groups.UiCloudWatchLogGroupimported but unused (F401)Remove unused import
8-8:
resources.pacbot_app.cloudwatch_log_groups.ApiCloudWatchLogGroupimported but unused (F401)Remove unused import
9-9:
resources.pacbot_app.ecr.APIEcrRepositoryimported but unused (F401)Remove unused import
9-9:
resources.pacbot_app.ecr.UIEcrRepositoryimported but unused (F401)Remove unused import
10-10: Redefinition of unused
AwsRegionfrom line 7 (F811)Remove definition:
AwsRegion
12-12: Redefinition of unused
ESDomainfrom line 6 (F811)Remove definition:
ESDomain
13-13:
resources.iam.ecs_role.ECSRoleimported but unused (F401)Remove unused import:
resources.iam.ecs_role.ECSRole
23-23:
resources.notification.function.SendNotificationFunctionimported but unused (F401)Remove unused import:
resources.notification.function.SendNotificationFunction
44-44: Use format specifiers instead of percent format (UP031)
Replace with format specifiers
61-61: Local variable
db_user_nameis assigned to but never used (F841)Remove assignment to unused variable
db_user_name
62-62: Local variable
db_passwordis assigned to but never used (F841)Remove assignment to unused variable
db_password
63-63: Local variable
db_hostis assigned to but never used (F841)Remove assignment to unused variable
db_host
222-223: Use format specifiers instead of percent format (UP031)
Replace with format specifiers
228-229: Use format specifiers instead of percent format (UP031)
Replace with format specifiers
234-235: Use format specifiers instead of percent format (UP031)
Replace with format specifiers
244-245: Use format specifiers instead of percent format (UP031)
Replace with format specifiers
251-252: Use format specifiers instead of percent format (UP031)
Replace with format specifiers
257-258: Use format specifiers instead of percent format (UP031)
Replace with format specifiers
Additional comments not posted (7)
installer/resources/pacbot_app/build_ui_and_api.py (1)
45-45: Ensure the new environment variable is correctly integrated and documented.This change integrates the
ENV_ASSET_GOVERNANCE_FEATURE_FLAGinto the environment setup for local executions, aligning with the PR's objectives to manage asset-related functionalities.installer/files/scripts/build_pacbot.py (2)
230-233: Ensure the dynamic setting ofassetGovernanceEnabledis correctly implemented.The
assetGovernanceEnabledconfiguration is properly updated based on theasset_governance_enabledattribute.
295-295: Verify that the environment variable is correctly fetched and used.Verification successful
The
ASSET_GOVERNANCE_FEATURE_FLAGenvironment variable is correctly fetched and used in the script. The references found in the codebase indicate that this variable is consistently used across multiple files.
installer/settings/default.local.py: Defines the flag with a default value.installer/resources/pacbot_app/build_ui_and_api.py: Passes the flag to the environment settings.installer/resources/pacbot_app/import_db.py: Uses the flag in the import database script.installer/files/scripts/build_pacbot.py: Fetches the flag from the environment.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correct usage of `ASSET_GOVERNANCE_FEATURE_FLAG` environment variable. # Test: Search for the environment variable usage. Expect: Correct usage in the script. rg --type python $'ASSET_GOVERNANCE_FEATURE_FLAG'Length of output: 79
Script:
#!/bin/bash # Description: Verify the correct usage of `ASSET_GOVERNANCE_FEATURE_FLAG` environment variable. # Test: Search for the environment variable usage. Expect: Correct usage in the script. rg --type py $'ASSET_GOVERNANCE_FEATURE_FLAG'Length of output: 522
installer/resources/pacbot_app/import_db.py (1)
188-189: Ensure the new environment variableASSET_GOVERNANCE_FEATURE_FLAGis correctly integrated and documented.The integration of the
ASSET_GOVERNANCE_FEATURE_FLAGinto the environment setup for database operations aligns with the PR's objectives to manage asset-related functionalities.installer/resources/pacbot_app/files/DB.sql (3)
128-128: The creation of theOmniSearch_Configtable appears unrelated to the feature flag implementation. Please clarify if this is intentional.
128-128: Ensure that theASSET_GOVERNANCE_FEATURE_FLAGvariable is properly used in subsequent SQL queries.
3366-3368: Ensure that the insert operation foraws.eventbridge.bus.detailswith theASSET_GOVERNANCE_FEATURE_FLAGis correctly implemented and has the intended effect.Verification successful
The insert operation for
aws.eventbridge.bus.detailswith theASSET_GOVERNANCE_FEATURE_FLAGis correctly implemented and has the intended effect.
installer/resources/pacbot_app/files/DB.sql: The variableASSET_GOVERNANCE_FEATURE_FLAGis defined and used correctly in the insert statement.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of the insert operation involving `ASSET_GOVERNANCE_FEATURE_FLAG`. # Test: Search for the insert operation and verify its correctness. rg --type sql $'INSERT IGNORE INTO pac_config_properties' | grep 'ASSET_GOVERNANCE_FEATURE_FLAG'Length of output: 421
Script:
#!/bin/bash # Description: Verify the definition and usage of the `ASSET_GOVERNANCE_FEATURE_FLAG` variable in the SQL files. # Test: Search for the definition of `ASSET_GOVERNANCE_FEATURE_FLAG`. rg --type sql 'ASSET_GOVERNANCE_FEATURE_FLAG'Length of output: 484
Description
Asset governance feature flag
Please include a summary of the changes and the related issues. Please also include relevant motivation and context. List
any dependencies that are required for this change.
Problem
Solution
Fixes # (issue if any)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also
list any relevant details for your test configuration
Checklist:
Other Information:
List any documentation updates that are needed for the Wiki
Summary by CodeRabbit
New Features
Settings
ASSET_GOVERNANCE_FEATURE_FLAGwith a default value ofFalsefor enhanced governance control.