T12828: Use the pywikibot-config repo for configuration #4071
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes introduced in this pull request enhance the Changes
Sequence Diagram(s)sequenceDiagram
participant Puppet as Puppet
participant YAML as YAML Config
participant Cron as Cron Jobs
participant UserConfig as User Config
participant Wikitide as Wikitide Family
Puppet->>YAML: Load $family_langs
Puppet->>YAML: Load $pwb_crons
YAML-->>Puppet: Return configurations
Puppet->>Cron: Create cron jobs from $pwb_crons
Cron-->>Puppet: Confirm cron jobs created
Puppet->>UserConfig: Update user-config.py
UserConfig-->>Puppet: Confirm updates
Puppet->>Wikitide: Generate dynamic language mappings
Wikitide-->>Puppet: Confirm mappings generated
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
redbluegreenhat
force-pushed
the
T12828
branch
2 times, most recently
from
3f6793a
to
c380ad7
Compare
|
@dependabot review |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (2)
modules/irc/templates/pywikibot/user-config.py (1)
1-1: Consider adding more detailed configuration documentation.Since this is a Puppet-managed file with significant changes to configuration patterns, it would be helpful to add comments explaining:
- The purpose of using wildcard configurations
- Any limitations or considerations for future maintainers
Add documentation like this:
-# MANAGED BY PUPPET +# MANAGED BY PUPPET +# This configuration uses wildcard ('*') patterns to support dynamic wiki configurations +# loaded from the pywikibot-config repository.modules/puppetserver/manifests/init.pp (1)
91-98: Implementation looks good, consider adding supporting configurations.The git::clone resource is properly configured with correct permissions and dependencies. However, consider adding the following supporting configurations to align with other critical repositories:
- Add an auto-update cron job similar to puppet-git and ssl-git
- Include the directory in backup configurations if the content is critical
Consider adding the following configurations:
+ cron { 'pywikibot-config-git': + command => '/usr/bin/git -C /etc/puppetlabs/puppet/pywikibot-config pull > /dev/null 2>&1', + user => 'root', + hour => '*', + minute => [ '9', '19', '29', '39', '49', '59' ], + }If the configurations are critical, also consider adding to the private backup cron job:
cron { 'backups-private': ensure => present, - command => '/usr/local/bin/wikitide-backup backup private > /var/log/private-backup.log 2>&1', + command => '/usr/local/bin/wikitide-backup backup private,pywikibot-config > /var/log/private-backup.log 2>&1', user => 'root', minute => '0', hour => '3', weekday => '0', }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
modules/irc/manifests/pywikibot.pp(2 hunks)modules/irc/templates/pywikibot/user-config.py(1 hunks)modules/irc/templates/pywikibot/wikitide_family.py(1 hunks)modules/puppetserver/manifests/init.pp(1 hunks)
🧰 Additional context used
🪛 Ruff
modules/irc/templates/pywikibot/user-config.py
4-4: SyntaxError: Expected a statement
4-5: SyntaxError: Expected a statement
modules/irc/templates/pywikibot/wikitide_family.py
18-18: SyntaxError: Expected an expression
18-18: SyntaxError: Expected an expression
18-18: SyntaxError: Got unexpected token $
18-18: SyntaxError: Got unexpected token $
18-18: SyntaxError: Expected an expression
18-18: SyntaxError: Expected an expression
20-20: SyntaxError: Expected an expression or a '}'
20-20: SyntaxError: Expected an expression or a '}'
20-20: SyntaxError: Expected an expression
20-20: SyntaxError: Expected an expression
21-21: SyntaxError: Expected an expression
25-25: SyntaxError: Expected an expression
25-25: SyntaxError: Expected an expression
25-25: SyntaxError: Got unexpected token $
25-25: SyntaxError: Got unexpected token $
25-25: SyntaxError: Expected an expression
25-25: SyntaxError: Expected an expression
27-27: SyntaxError: Expected an expression or a '}'
27-27: SyntaxError: Expected an expression or a '}'
27-27: SyntaxError: Expected an expression
27-27: SyntaxError: Expected an expression
28-28: SyntaxError: Expected an expression
32-32: SyntaxError: Expected an expression
32-32: SyntaxError: Expected an expression
32-32: SyntaxError: Got unexpected token $
32-32: SyntaxError: Got unexpected token $
32-32: SyntaxError: Expected an expression
32-32: SyntaxError: Expected an expression
34-34: SyntaxError: Expected an expression or a '}'
34-34: SyntaxError: Expected an expression or a '}'
34-34: SyntaxError: Expected an expression
34-34: SyntaxError: Expected an expression
35-35: SyntaxError: Expected an expression
🔇 Additional comments (7)
modules/irc/templates/pywikibot/user-config.py (1)
3-5: Verify the impact of using wildcard configurations.
The changes from specific keys ('meta', '.miraheze.org') to wildcard keys ('') make the configuration more generic. While this aligns with the dynamic configuration approach:
- Ensure that using '*' for
mylangdoesn't affect wiki-specific operations - Verify that using '*' for authentication is secure and won't grant unintended access
Let's check for any security implications or specific wiki configurations:
✅ Verification successful
Let me gather more information about the pywikibot configuration and its usage:
The additional context will help verify if there are any other pywikibot configurations or dependencies that might be affected by the wildcard changes.
Using wildcard configurations is safe in this context
The verification shows that:
- The wildcard authentication ('*') is used only for this specific bot (BeeBot) and the credentials are properly templated using Puppet variables
- The family file (wikitide_family.py) properly defines the wiki configuration with specific domains and protocols
- No other pywikibot configurations or scripts in the codebase are affected by or dependent on specific wiki configurations
The changes align with the bot's purpose of operating across the WikiTide infrastructure, and the authentication scope is properly controlled through the bot's credentials rather than the wildcard configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any wiki-specific configurations that might be affected
rg -A 3 "mylang\s*=\s*['\"](meta|metawiki)['\"]"
# Look for any security-related patterns in authentication
rg -A 5 "authenticate\[.*miraheze\.org.*\]"
# Check for any references to specific wikis in the pywikibot config
fd -e py -e yaml . | xargs rg "meta\.miraheze\.org"
Length of output: 1630
Script:
#!/bin/bash
# Check for any other pywikibot config files and their language settings
fd pywikibot . | xargs rg "mylang|usernames|authenticate"
# Check for any imports or usage of this config file
rg -l "import.*user-config" --type py
# Look for any other authentication patterns in pywikibot files
fd -e py . | xargs rg "authenticate\[.*\]"
# Check for any family file configurations
fd family.py . | xargs cat
Length of output: 1495
🧰 Tools
🪛 Ruff
4-4: SyntaxError: Expected a statement
4-5: SyntaxError: Expected a statement
modules/irc/templates/pywikibot/wikitide_family.py (3)
11-16: LGTM: Clean class definition following PyWikiBot patterns
The class definition and inheritance are implemented correctly.
30-35: LGTM: Enforcing HTTPS is a good security practice
The hardcoded 'https' protocol is appropriate as it enforces secure connections for all wikis.
🧰 Tools
🪛 Ruff
32-32: SyntaxError: Expected an expression
32-32: SyntaxError: Expected an expression
32-32: SyntaxError: Got unexpected token $
32-32: SyntaxError: Got unexpected token $
32-32: SyntaxError: Expected an expression
32-32: SyntaxError: Expected an expression
34-34: SyntaxError: Expected an expression or a '}'
34-34: SyntaxError: Expected an expression or a '}'
34-34: SyntaxError: Expected an expression
34-34: SyntaxError: Expected an expression
35-35: SyntaxError: Expected an expression
17-21: 🛠️ Refactor suggestion
Consider adding validation for the family_langs configuration
While the template structure is correct, consider adding validation to handle edge cases:
- Empty family_langs variable
- Domain names with special characters
Let's verify the family_langs configuration structure:
Consider adding validation in the template:
langs = {
+ <% if @family_langs.empty? -%>
+ 'default': 'wikitide.org', # Fallback domain
+ <% else -%>
<% family_langs.each |$dbname, $params| {-%>
'<%= $dbname %>': '<%= $params[domain] %>',
<% } -%>
+ <% end -%>
}🧰 Tools
🪛 Ruff
18-18: SyntaxError: Expected an expression
18-18: SyntaxError: Expected an expression
18-18: SyntaxError: Got unexpected token $
18-18: SyntaxError: Got unexpected token $
18-18: SyntaxError: Expected an expression
18-18: SyntaxError: Expected an expression
20-20: SyntaxError: Expected an expression or a '}'
20-20: SyntaxError: Expected an expression or a '}'
20-20: SyntaxError: Expected an expression
20-20: SyntaxError: Expected an expression
21-21: SyntaxError: Expected an expression
modules/irc/manifests/pywikibot.pp (2)
97-107: Review the use of lint ignore directives.
The lint:ignore:selector_inside_resource directive suggests potential Puppet style issues. Consider restructuring to avoid the need for lint ignores.
Run this script to check for similar patterns in the codebase:
#!/bin/bash
# Description: Find similar lint ignore patterns that might need refactoring
# Test: Search for lint ignore directives
rg "lint:ignore:selector_inside_resource" -A 582-83: Verify YAML file existence and add error handling.
The YAML loading lacks error handling for cases where the file might be missing or malformed. This could lead to silent failures or unexpected behavior.
Consider:
- Adding a check for file existence before loading
- Implementing error handling for malformed YAML
- Adding validation for required language configurations
Run this script to verify the file existence and permissions:
modules/puppetserver/manifests/init.pp (1)
91-98: Verify repository existence and integration.
Let's verify the repository setup and its integration with the irc::pywikibot class.
✅ Verification successful
Let me check the actual integration in the irc::pywikibot class to verify how this configuration is being used.
Repository and integration verified successfully
The verification confirms:
- The repository exists at github.com/miraheze/pywikibot-config and contains the required configuration
- The integration is properly implemented in irc::pywikibot class, which:
- Loads language configurations from
/etc/puppetlabs/puppet/pywikibot-config/langs.yaml - Loads cron configurations from
/etc/puppetlabs/puppet/pywikibot-config/cron.yaml
- Loads language configurations from
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify repository existence and integration
# Test 1: Verify the repository exists and is accessible
echo "Verifying repository existence..."
gh repo view miraheze/pywikibot-config
# Test 2: Check for references to this configuration in irc::pywikibot
echo "Checking integration with irc::pywikibot..."
rg -l "pywikibot-config" modules/irc/manifests/
Length of output: 624
Script:
#!/bin/bash
# Check the content of irc::pywikibot class to understand the integration
rg -A 5 -B 5 "pywikibot-config" modules/irc/manifests/pywikibot.pp
Length of output: 894
modules/irc/manifests/pywikibot.pp
Outdated
| cron { "pywikibot ${dbname}-${script}": | ||
| ensure => $ensure, | ||
| command => "/usr/local/bin/pywikibot ${script} ${scriptparams} -lang:${dbname} -pt:0 >> /var/log/pwb/${dbname}-${script}-cron.log", | ||
| user => 'irc', | ||
| minute => $minute, | ||
| hour => $hour, | ||
| month => $month, | ||
| monthday => $monthday, | ||
| } |
There was a problem hiding this comment.
Validate script parameters and add error handling.
The cron command construction could be vulnerable to injection if the YAML configuration isn't properly validated. Additionally, the hardcoded -pt:0 parameter might need to be configurable.
Consider:
- Adding parameter validation
- Making the
-pt:0parameter configurable - Escaping or sanitizing
scriptparams
Example validation:
if !($scriptparams =~ /^[a-zA-Z0-9\s-]*$/) {
fail("Invalid script parameters for ${dbname}-${script}")
}There was a problem hiding this comment.
no need to make -pt configurable.
There was a problem hiding this comment.
@redbluegreenhat, understood. I'll note that the -pt parameter does not need to be configurable.
✏️ Learnings added
Learnt from: redbluegreenhat
PR: miraheze/puppet#4071
File: modules/irc/manifests/pywikibot.pp:98-106
Timestamp: 2024-11-02T10:17:27.076Z
Learning: In the `irc::pywikibot` class in `modules/irc/manifests/pywikibot.pp`, the `-pt:0` parameter in the cron command does not need to be configurable.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Can you also validate script and dbname here? They should only contain lowercase letters, no spaces or numbers at all.
There was a problem hiding this comment.
@redbluegreenhat, certainly! We can add validation for script and dbname to ensure they only contain lowercase letters with no spaces or numbers. Here's how you could implement the validation:
if !($script =~ /^[a-z]+$/) {
fail("Invalid script name '${script}' for database '${dbname}'. Script names must only contain lowercase letters without spaces or numbers.")
}
if !($dbname =~ /^[a-z]+$/) {
fail("Invalid database name '${dbname}'. Database names must only contain lowercase letters without spaces or numbers.")
}These checks will ensure that any invalid script or dbname values will cause the Puppet run to fail with a descriptive error message.
✏️ Learnings added
Learnt from: redbluegreenhat
PR: miraheze/puppet#4071
File: modules/irc/manifests/pywikibot.pp:0-0
Timestamp: 2024-11-08T15:26:53.589Z
Learning: In the `irc::pywikibot` class in `modules/irc/manifests/pywikibot.pp`, validate that `script` and `dbname` contain only lowercase letters with no spaces or numbers.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
oops actually dbname can contain numbers, my bad. Please modify your code appropriately for this.
There was a problem hiding this comment.
@redbluegreenhat, understood! Here's the updated validation code that allows numbers in dbname:
# Validate that script contains only lowercase letters with no spaces or numbers
if !($script =~ /^[a-z]+$/) {
fail("Invalid script name '${script}' for database '${dbname}'. Script names must only contain lowercase letters without spaces or numbers.")
}
# Validate that dbname contains only lowercase letters and numbers with no spaces
if !($dbname =~ /^[a-z0-9]+$/) {
fail("Invalid database name '${dbname}'. Database names must only contain lowercase letters and numbers without spaces.")
}This ensures that dbname can contain numbers while script remains restricted to lowercase letters without numbers or spaces.
✏️ Learnings added
Learnt from: redbluegreenhat
PR: miraheze/puppet#4071
File: modules/irc/manifests/pywikibot.pp:0-0
Timestamp: 2024-11-08T15:26:53.589Z
Learning: In the `irc::pywikibot` class in `modules/irc/manifests/pywikibot.pp`, `dbname` can contain numbers, while `script` should only contain lowercase letters with no spaces or numbers.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
redbluegreenhat
force-pushed
the
T12828
branch
6 times, most recently
from
58388e9
to
ca754c6
Compare
redbluegreenhat
force-pushed
the
T12828
branch
2 times, most recently
from
46406bd
to
70641eb
Compare
redbluegreenhat
force-pushed
the
T12828
branch
from
70641eb
to
09024c7
Compare
redbluegreenhat
force-pushed
the
T12828
branch
4 times, most recently
from
78c162c
to
4c271cf
Compare
redbluegreenhat
force-pushed
the
T12828
branch
from
4c271cf
to
ef4a0e3
Compare
|
Validation of the parameters that will end up on the command on the cron should be improved before merging, as well as having better checks on the CI at pywikibot-config. |
https://issue-tracker.miraheze.org/T12828