script to facilitate resetting wikis (only partial) #3854
Conversation
WalkthroughEnhancements to the Changes
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: 3
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- modules/salt/files/bin/partial-reset-wiki.py (1 hunks)
Additional Context Used
Ruff (1)
modules/salt/files/bin/partial-reset-wiki.py (1)
62-62: SyntaxError: Multiple exception types must be parenthesized
Additional comments not posted (4)
modules/salt/files/bin/partial-reset-wiki.py (4)
10-16: TheDbClusterMapclass is well-defined and matches the description in the summary.
18-24: The global variabledb_clustersis correctly initialized with mappings. Ensure that all clusters (c1toc5) are utilized as needed throughout the script.
27-28: The functiongenerate_salt_commandis concise and correctly generates a SaltStack command. Good use of f-string for command construction.
73-79: Themainfunction is well-structured and correctly parses command-line arguments. Ensure that the--wikiargument is used consistently and correctly throughout the script.Verification successful
Consistent Use of
--wikiArgument VerifiedThe
--wikiargument is used consistently throughout the script and other related scripts.
modules/salt/files/bin/partial-reset-wiki.py: Line 73modules/salt/files/bin/rename-wiki.py: Line 181modules/mediawiki/files/bin/test_mwscript.py: Multiple linesmodules/mediawiki/files/bin/mwdeploy.py: Multiple linesmodules/mediawiki/files/bin/mwscript.py: Multiple linesScripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the consistent use of the `--wiki` argument. # Test: Search for the usage of `--wiki` argument. rg --type python $'--wiki'Length of output: 434
Script:
#!/bin/bash # Description: Verify the consistent use of the `--wiki` argument. # Test: Search for the usage of `--wiki` argument. rg --type py '--wiki'Length of output: 430
Script:
#!/bin/bash # Description: Verify the consistent use of the `--wiki` argument. # Test: Search for the usage of `--wiki` argument. rg --type py '\-\-wiki'Length of output: 6385
| def execute_salt_command(salt_command: str, shell: bool = True, stdout: Optional[int] = None, text: Optional[bool] = None) -> Optional[subprocess.CompletedProcess]: | ||
| response = input(f'EXECUTE (type c(continue), s(kip), a(bort): {salt_command}') | ||
| if response in ['c', 'continue']: | ||
| return subprocess.run(salt_command, shell=shell, stdout=stdout, text=text) | ||
| if response in ['s', 'skip']: | ||
| return None | ||
| sys.exit(1) # noqa: R503 |
There was a problem hiding this comment.
Tip
Codebase Verification
Handle potential exceptions from subprocess.run in execute_salt_command
The execute_salt_command function currently does not handle potential exceptions from subprocess.run. It is recommended to wrap the subprocess.run call in a try-except block to handle possible exceptions such as subprocess.CalledProcessError.
Suggested Change:
def execute_salt_command(salt_command: str, shell: bool = True, stdout: Optional[int] = None, text: Optional[bool] = None) -> Optional[subprocess.CompletedProcess]:
response = input(f'EXECUTE (type c(continue), s(kip), a(bort): {salt_command}')
if response in ['c', 'continue']:
try:
return subprocess.run(salt_command, shell=shell, stdout=stdout, text=text, check=True)
except subprocess.CalledProcessError as e:
print(f"Command '{salt_command}' failed with return code {e.returncode}")
except Exception as e:
print(f"An error occurred: {e}")
return None
if response in ['s', 'skip']:
return None
sys.exit(1) # noqa: R503- modules/salt/files/bin/partial-reset-wiki.py: Add exception handling around
subprocess.run.
Analysis chain
The execute_salt_command function introduces a user interaction to decide whether to continue, skip, or abort the command execution. This is a good safety feature but consider handling potential exceptions from subprocess.run.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify proper exception handling in subprocess.run usage.
# Test: Search for exception handling patterns around subprocess.run.
rg --type python $'subprocess.run'
Length of output: 64
Script:
#!/bin/bash
# Description: Verify proper exception handling in subprocess.run usage.
# Test: Search for exception handling patterns around subprocess.run.
rg 'subprocess.run' --glob '*.py'
Length of output: 362
Script:
#!/bin/bash
# Description: Extract and review the context around subprocess.run usage in partial-reset-wiki.py.
# Extract 5 lines before and after the subprocess.run usage in partial-reset-wiki.py
rg 'subprocess.run' -A 5 -B 5 modules/salt/files/bin/partial-reset-wiki.py
Length of output: 532
There was a problem hiding this comment.
This is mostly useful. I disagree that a fatal should return None though. We should probably abort.
There was a problem hiding this comment.
@RhinosF1, thank you for your feedback. I agree that aborting the script in the case of a fatal error is a safer approach. Here's the revised suggestion for the execute_salt_command function to handle exceptions more robustly:
def execute_salt_command(salt_command: str, shell: bool = True, stdout: Optional[int] = None, text: Optional[bool] = None) -> Optional[subprocess.CompletedProcess]:
response = input(f'EXECUTE (type c(continue), s(kip), a(bort): {salt_command}')
if response in ['c', 'continue']:
try:
return subprocess.run(salt_command, shell=shell, stdout=stdout, text=text, check=True)
except subprocess.CalledProcessError as e:
print(f"Command '{salt_command}' failed with return code {e.returncode}")
sys.exit(1) # Abort on fatal error
except Exception as e:
print(f"An error occurred: {e}")
sys.exit(1) # Abort on any other critical error
if response in ['s', 'skip']:
return None
sys.exit(1) # Abort on user requestThis modification ensures that the script will abort if a critical error occurs during the execution of a command, which should prevent any unstable state from continuing. Let me know if this aligns with your expectations or if further adjustments are needed.
There was a problem hiding this comment.
This would be a good idea
There was a problem hiding this comment.
@RhinosF1, I'm glad to hear that the revised approach meets your expectations! If there are any more adjustments or further details you need, feel free to let me know. I'm here to help! 😊
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| def get_db_cluster(wiki: str) -> str: | ||
| db_query = f'SELECT wiki_dbcluster FROM mhglobal.cw_wikis WHERE wiki_dbname = \\"{wiki}\\"' | ||
| command = generate_salt_command("db171", f"sudo -i mysql --skip-column-names -e '{db_query}'") | ||
| result = execute_salt_command(salt_command=command, stdout=subprocess.PIPE, text=True) | ||
| if result: | ||
| cluster_name = result.stdout.strip() | ||
| #print(cluster_name) | ||
| cluster_data = cluster_name.split('\n') | ||
| cluster_data_b = cluster_data[1].split(' ') | ||
| print(cluster_data_b) | ||
| #print("Extracted cluster_name:", cluster_name) # Print cluster_name for debugging | ||
| cluster_name = cluster_data_b[4] | ||
|
|
||
| return db_clusters[cluster_name] # type: ignore[literal-required] |
There was a problem hiding this comment.
Tip
Codebase Verification
SQL Query Format and Database Interaction Security
The SQL query in the get_db_cluster function uses string interpolation, which can be insecure and prone to SQL injection attacks. It is recommended to use parameterized queries to enhance security.
Simplification of Extraction Logic
The extraction of cluster_name from result.stdout is complex and error-prone. Simplifying this logic can improve code readability and maintainability.
Recommendations
- Use parameterized queries to prevent SQL injection.
- Simplify the extraction logic for
cluster_name.
Suggested Changes
import mysql.connector
def get_db_cluster(wiki: str) -> str:
db_query = "SELECT wiki_dbcluster FROM mhglobal.cw_wikis WHERE wiki_dbname = %s"
command = generate_salt_command("db171", f"sudo -i mysql --skip-column-names -e '{db_query}'")
result = execute_salt_command(salt_command=command, stdout=subprocess.PIPE, text=True)
if result:
cluster_name = result.stdout.strip()
cluster_data = cluster_name.split('\n')
cluster_data_b = cluster_data[1].split(' ')
print(cluster_data_b)
cluster_name = cluster_data_b[1] # Assuming the cluster name is at index 1
return db_clusters[cluster_name] # type: ignore[literal-required]- Use parameterized queries: Modify the SQL query to use parameters instead of string interpolation.
- Simplify extraction logic: Adjust the index used to extract
cluster_namebased on the actual data structure.
Locations to Update
modules/salt/files/bin/partial-reset-wiki.py
Analysis chain
The function get_db_cluster uses a SQL query to fetch the cluster name. Ensure that the SQL query is correctly formatted and that the database interaction is secure. The extraction of cluster_name from result.stdout seems overly complex and error-prone.
- cluster_name = cluster_data_b[4]
+ cluster_name = cluster_data_b[1] # Assuming the cluster name is at index 1Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the SQL query format and database interaction security.
# Test: Search for SQL query patterns and security practices.
rg --type python $'SELECT'
Length of output: 56
Script:
#!/bin/bash
# Description: Verify the SQL query format and database interaction security.
# Test: Search for SQL query patterns in Python files.
rg 'SELECT' --glob '*.py'
# Test: Extract the implementation of the get_db_cluster function to analyze its structure.
ast-grep --lang python --pattern $'def get_db_cluster($_):\n $$$'
Length of output: 428
There was a problem hiding this comment.
The additional comment misses the context completely because AI.
The risk of SQLi is low. Anyone running this has root anyone so input should be trusted.
There was a problem hiding this comment.
New Learnings Added
User: RhinosF1
PR: miraheze/puppet#3854
File: modules/salt/files/bin/partial-reset-wiki.py:40-53
Timestamp: 2024-05-18T08:42:28.420Z
Learning: @RhinosF1, thank you for your insights. I understand that the script is intended to be run in a controlled environment by trusted users with root access, which indeed lowers the risk of malicious input leading to SQL injection.
However, adopting best practices such as parameterized queries can still be beneficial. It not only guards against potential accidental mishaps but also enhances the maintainability and robustness of the code. This approach can be particularly useful in scenarios where the script might be modified or extended in the future, possibly in ways that could change the assumptions about the security context.
That said, I appreciate the context you've provided, and it's crucial for tailoring the security measures appropriately. If there are any more specific aspects of the script or its environment that you think should be considered in the review, please let me know!
<!--
The `partial-reset-wiki.py` script is intended to be run in a controlled environment by trusted users with root access, which lowers the risk of malicious input leading to SQL injection.
Note: Learnings are effective only in the context of specific code segments. To apply general coding guidelines, please configure review instructions.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| def reset_wiki(wiki: str) -> None: | ||
| # Step 1: Get the db cluster for the wiki | ||
|
|
||
| try: | ||
| wiki_cluster = get_db_cluster(wiki_db) | ||
| except KeyError, IndexError: | ||
| print(f'Error: Unable to determine the db cluster for {wiki}') | ||
| sys.exit(1) | ||
|
|
||
| # Step 2: Execute deleteWiki.php | ||
| execute_salt_command(salt_command=generate_salt_command('mwtask181', f'mwscript extensions/CreateWiki/deleteWiki.php loginwiki --deletewiki {wiki} --delete {os.getlogin()}')) | ||
|
|
||
| # Step 3: Backup and drop database | ||
| execute_salt_command(salt_command=generate_salt_command(wiki_cluster, f"sudo -i mysqldump {wiki} > {wiki}.sql'")) | ||
| execute_salt_command(salt_command=generate_salt_command(wiki_cluster, f"sudo -i mysql -e 'DROP DATABASE {wiki}'")) | ||
|
|
There was a problem hiding this comment.
The reset_wiki function orchestrates the wiki reset process. However, there's a syntax error in handling multiple exceptions on line 62. Also, consider simplifying the command generation and execution process to reduce redundancy and improve readability.
- except KeyError, IndexError:
+ except (KeyError, IndexError):- execute_salt_command(salt_command=generate_salt_command('mwtask181', f'mwscript extensions/CreateWiki/deleteWiki.php loginwiki --deletewiki {wiki} --delete {os.getlogin()}'))
+ delete_command = generate_salt_command('mwtask181', f'mwscript extensions/CreateWiki/deleteWiki.php loginwiki --deletewiki {wiki} --delete {os.getlogin()}')
+ execute_salt_command(salt_command=delete_command)There was a problem hiding this comment.
I'm not sure this is true
There was a problem hiding this comment.
@RhinosF1, thank you for your comment. Let's clarify the points raised:
-
Syntax Correction: The original syntax
except KeyError, IndexError:is incorrect in Python for handling multiple exceptions. The correct syntax should use parentheses:except (KeyError, IndexError):. This change ensures that the code correctly catches eitherKeyErrororIndexError, which might occur during the execution of theget_db_clusterfunction. -
Refactoring Command Execution: The suggestion to separate the command generation from its execution (
generate_salt_commandandexecute_salt_command) aims to improve the readability and maintainability of the code. By defining the command in a separate variable before execution, it becomes easier to manage and modify the command string in isolation from its execution logic. This practice can be particularly useful for debugging or when changes to the command structure are needed.
These changes are intended to enhance the code's clarity and robustness. If there's a specific part of the suggestion that seems incorrect or if there's more context that might affect these changes, please let me know!
There was a problem hiding this comment.
#1 is done now
#2 I don't have an opinion on
There was a problem hiding this comment.
@RhinosF1, thank you for updating the syntax for exception handling in the reset_wiki function. It's great to see that enhancement in place!
Regarding the refactoring of the command execution process, I understand your neutral stance. To provide a bit more context, separating the command generation from its execution can make the code easier to debug and modify in the future. It allows for clearer separation of concerns, where the command's structure is defined independently from where it's executed. This separation can be particularly beneficial if the command needs to be reused or adjusted, as it isolates the command string from the execution logic.
If you decide to implement this change and need any assistance or further clarification, feel free to reach out!
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
|
I'm really not a fan of having an LLM do code review here. It additionally draws into question whether you used an LLM like Copilot to facilitate making the script, since I additionally would not be happy having code from Copilot here, especially in a script that isn't finished. |
|
|
Yeah, I would strongly advise just ditching the entire thing. It doesn't look like it's provided any useful feedback and it's mostly just spamming the PR with fluffy prose. |
There's a lot of fluff but it does have some actual genuinely useful suggestions to it. I am going to do a human review of this code later. |
|
Also will apologise for my miscommunication here: because this was just in this PR, I thought it was a user-triggered bot and not a new repo config. I do genuinely have a serious (and well-founded) distrust of anything that advertises itself as "AI," since LLMs have been found to only be effective at wasting resources. However, specifically for this tool, it suffers the primary drawbacks of something designed to use LLMs, which is everything being written like a high-school essay needing to meet a word count quota. It provides trivial feedback at best but surrounds all of its feedback in a massive pillow of prose that needs to be sifted through first, and it's just a bunch of spam to sift through when human feedback can be much more concise. |
|
I don't disagree that there's drawbacks. You might benefit from joining the #miraheze-tech & #miraheze-tech-ops channels on libera.chat or https://mirhaeze.org/discord - it might help you gain more of an insight and get involved. |
|
I have been in the Discord and have actually wanted to get more involved; it's why I've been trying to at least review PRs here so I can get a better idea of what tasks are being done. I just haven't kept up on it since I haven't had the time, and I know a lot goes on in these channels. |
|
|
||
| # Step 3: Backup and drop database | ||
| execute_salt_command(salt_command=generate_salt_command(wiki_cluster, f"sudo -i mysqldump {wiki} > {wiki}.sql'")) | ||
| execute_salt_command(salt_command=generate_salt_command(wiki_cluster, f"sudo -i mysql -e 'DROP DATABASE {wiki}'")) |
There was a problem hiding this comment.
Should we add some logging here?
There was a problem hiding this comment.
Yes, it should be the same as with the rename script. This was just the first draft so I could test things out.
|
|
||
| def main() -> None: | ||
| parser = argparse.ArgumentParser(description='Executes the commands needed to reset wikis') | ||
| parser.add_argument('--wiki', required=True, help='Old wiki database name') |
There was a problem hiding this comment.
Copied from the other script?
please correct the description
| def reset_wiki(wiki: str) -> None: | ||
| # Step 1: Get the db cluster for the wiki | ||
|
|
||
| try: | ||
| wiki_cluster = get_db_cluster(wiki_db) | ||
| except KeyError, IndexError: | ||
| print(f'Error: Unable to determine the db cluster for {wiki}') | ||
| sys.exit(1) | ||
|
|
||
| # Step 2: Execute deleteWiki.php | ||
| execute_salt_command(salt_command=generate_salt_command('mwtask181', f'mwscript extensions/CreateWiki/deleteWiki.php loginwiki --deletewiki {wiki} --delete {os.getlogin()}')) | ||
|
|
||
| # Step 3: Backup and drop database | ||
| execute_salt_command(salt_command=generate_salt_command(wiki_cluster, f"sudo -i mysqldump {wiki} > {wiki}.sql'")) | ||
| execute_salt_command(salt_command=generate_salt_command(wiki_cluster, f"sudo -i mysql -e 'DROP DATABASE {wiki}'")) | ||
|
|
There was a problem hiding this comment.
I'm not sure this is true
| def get_db_cluster(wiki: str) -> str: | ||
| db_query = f'SELECT wiki_dbcluster FROM mhglobal.cw_wikis WHERE wiki_dbname = \\"{wiki}\\"' | ||
| command = generate_salt_command("db171", f"sudo -i mysql --skip-column-names -e '{db_query}'") | ||
| result = execute_salt_command(salt_command=command, stdout=subprocess.PIPE, text=True) | ||
| if result: | ||
| cluster_name = result.stdout.strip() | ||
| #print(cluster_name) | ||
| cluster_data = cluster_name.split('\n') | ||
| cluster_data_b = cluster_data[1].split(' ') | ||
| print(cluster_data_b) | ||
| #print("Extracted cluster_name:", cluster_name) # Print cluster_name for debugging | ||
| cluster_name = cluster_data_b[4] | ||
|
|
||
| return db_clusters[cluster_name] # type: ignore[literal-required] |
There was a problem hiding this comment.
The additional comment misses the context completely because AI.
The risk of SQLi is low. Anyone running this has root anyone so input should be trusted.
| def execute_salt_command(salt_command: str, shell: bool = True, stdout: Optional[int] = None, text: Optional[bool] = None) -> Optional[subprocess.CompletedProcess]: | ||
| response = input(f'EXECUTE (type c(continue), s(kip), a(bort): {salt_command}') | ||
| if response in ['c', 'continue']: | ||
| return subprocess.run(salt_command, shell=shell, stdout=stdout, text=text) | ||
| if response in ['s', 'skip']: | ||
| return None | ||
| sys.exit(1) # noqa: R503 |
There was a problem hiding this comment.
This is mostly useful. I disagree that a fatal should return None though. We should probably abort.
<!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Chores** - Updated code quality checks to include additional modules for flake8. - **Refactor** - Refactored the `partial-reset-wiki.py` script to improve code quality and functionality. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- .github/workflows/python.yml (1 hunks)
- modules/salt/files/bin/partial-reset-wiki.py (1 hunks)
Files skipped from review due to trivial changes (1)
- .github/workflows/python.yml
Files skipped from review as they are similar to previous changes (1)
- modules/salt/files/bin/partial-reset-wiki.py
Summary by CodeRabbit
modules/salt/files/bindirectory.