Update rippled.cfg file #824
Conversation
WalkthroughThe changes include a cleanup and update of the Changes
Sequence Diagram(s)sequenceDiagram
participant PR as "Pull Request"
participant CI as "GitHub Actions"
participant Script as "Fetch Amendments Script"
participant Server as "Remote Server"
PR->>CI: Open Pull Request
CI->>Script: Run "Fetch latest rippled amendments"
Script->>Server: HTTP GET request for amendments
Server-->>Script: Return amendments data
Script->>CI: Update rippled.cfg if changes found
CI->>CI: Run "Add and commit rippled.cfg" step
CI->>CI: Execute Docker step in background
sequenceDiagram
participant Script as "Fetch Amendments Script"
participant Regex as "Regex Extraction"
participant Config as "rippled.cfg"
Script->>Script: Fetch amendments from remote source
Script->>Regex: Extract amendments and fixes
Regex-->>Script: Return parsed amendment list
Script->>Config: Compare and update configuration if new entries exist
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (6)
🪧 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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
tools/fetch_rippled_amendments.py (1)
67-69: Consider handling ConfigParser limitations.As noted in the PR objectives, ConfigParser doesn't preserve comments. Consider adding a warning that comments will be removed or implementing a more robust solution.
with open(CONFIG_FILE, "w", encoding="utf-8") as rippled_cfg_file: + print("WARNING: ConfigParser will remove comments from the file.") config.write(rippled_cfg_file)Alternatively, you could implement a more robust solution using a different library or approach that preserves comments.
.ci-config/rippled.cfg (1)
54-54: Inconsistent JSON syntax in rpc_startup section.The configuration uses an equals sign for "command" but a colon for "severity", which is inconsistent.
-{ "command" = "log_level", "severity": "info" } +{ "command": "log_level", "severity": "info" }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.ci-config/rippled.cfg(3 hunks).github/workflows/integration_test.yml(2 hunks)tools/fetch_rippled_amendments.py(1 hunks)
🔇 Additional comments (6)
.github/workflows/integration_test.yml (4)
25-27: Good addition of PR-specific checkout parameters.The workflow now properly checks out the code from the pull request's repository and branch, which is essential for the commit step to work correctly later in the workflow.
55-59: Good implementation of the amendment fetching step.This step correctly executes the new Python script and only runs during pull requests, which aligns with the PR objectives.
60-70: Proper implementation of automated commit step.The workflow uses the EndBug/add-and-commit action to automatically commit any changes to the rippled.cfg file. The commit message and author details are appropriately set.
71-74: Strategic reordering of Docker step.Moving the Docker run step after the amendment fetching and commit steps ensures that the Docker container uses the updated configuration file.
.ci-config/rippled.cfg (2)
27-30: Good formatting improvements in node_db section.The spacing around equals signs improves readability and consistency.
142-149: Good addition of new amendments.These new amendments have been added correctly to the features section, which is the main purpose of the PR.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
tools/fetch_rippled_amendments.py (2)
52-54: Add null check for fetch_rippled_amendments result.Even though the function now returns an empty list on error, it's still good practice to validate the result before using it.
existing_amendments = [v for v in config[FEATURES_SECTION] if v] new_rippled_amendments = fetch_rippled_amendments() + + if not new_rippled_amendments: + print("WARNING: No amendments fetched. Using existing configuration.")
10-14: 🛠️ Refactor suggestionAdd error handling for network failures.
The script makes HTTP requests but doesn't handle network-related exceptions that could be raised by
requests.get().import configparser import os import re +import sys import requests CONFIG_FILE = os.path.join(os.getcwd(), ".ci-config", "rippled.cfg") FEATURES_SECTION = "features" RIPPLED_FEATURES_FILE = "https://raw.githubusercontent.com/XRPLF/rippled/develop/include/xrpl/protocol/detail/features.macro"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tools/fetch_rippled_amendments.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tools/fetch_rippled_amendments.py
37-37: SyntaxError: Expected ',', found name
43-43: SyntaxError: Expected ',', found name
43-44: SyntaxError: Expected ')', found newline
🪛 GitHub Actions: Integration test
tools/fetch_rippled_amendments.py
[error] 37-37: SyntaxError: invalid syntax
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Snippet test (3.11)
- GitHub Check: Snippet test (3.13)
- GitHub Check: Snippet test (3.10)
- GitHub Check: Snippet test (3.12)
- GitHub Check: Snippet test (3.8)
tools/fetch_rippled_amendments.py
Outdated
| def fetch_rippled_amendments(): | ||
| # Send a GET request | ||
| response = requests.get(RIPPLED_FEATURES_FILE, timeout=30) # 30 second timeout | ||
|
|
||
| # Check for successful request | ||
| if response.status_code == 200: | ||
| features_contents = response.text | ||
| feature_hits = re.findall( | ||
| feature_hits = re.findall( | ||
| r"^ *XRPL_FEATURE *\(([a-zA-Z0-9_]+), * Supported::yes, VoteBehavior::Default(Yes|No)", | ||
| features_contents, | ||
| re.MULTILINE, | ||
| ) | ||
|
|
||
| fix_hits = re.findall( | ||
| r"^ *XRPL_FIX *\(([a-zA-Z0-9_]+), * Supported::yes,", | ||
| features_contents, | ||
| re.MULTILINE, | ||
| ) | ||
|
|
||
| all_supported_amendments = feature_hits + ["fix" + f for f in fix_hits] | ||
| return all_supported_amendments | ||
| else: | ||
| print(f"Failed to fetch file. Status code: {response.status_code}") | ||
| return [] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add try-except block to handle request exceptions.
The current implementation doesn't handle exceptions that might be raised during the HTTP request, such as connection errors or timeouts.
def fetch_rippled_amendments():
# Send a GET request
- response = requests.get(RIPPLED_FEATURES_FILE, timeout=30) # 30 second timeout
-
- # Check for successful request
- if response.status_code == 200:
- features_contents = response.text
+ try:
+ response = requests.get(RIPPLED_FEATURES_FILE, timeout=30) # 30 second timeout
+
+ # Check for successful request
+ if response.status_code == 200:
+ features_contents = response.text
+ feature_hits = re.findall(
+ r"^ *XRPL_FEATURE *\(([a-zA-Z0-9_]+), * Supported::yes, VoteBehavior::Default(Yes|No)",
+ features_contents,
+ re.MULTILINE,
+ )
+
+ fix_hits = re.findall(
+ r"^ *XRPL_FIX *\(([a-zA-Z0-9_]+), * Supported::yes,",
+ features_contents,
+ re.MULTILINE,
+ )
+
+ all_supported_amendments = feature_hits + ["fix" + f for f in fix_hits]
+ return all_supported_amendments
+ else:
+ print(f"Failed to fetch file. Status code: {response.status_code}")
+ return []
+ except requests.exceptions.RequestException as e:
+ print(f"Error making request: {e}")
+ return []📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def fetch_rippled_amendments(): | |
| # Send a GET request | |
| response = requests.get(RIPPLED_FEATURES_FILE, timeout=30) # 30 second timeout | |
| # Check for successful request | |
| if response.status_code == 200: | |
| features_contents = response.text | |
| feature_hits = re.findall( | |
| feature_hits = re.findall( | |
| r"^ *XRPL_FEATURE *\(([a-zA-Z0-9_]+), * Supported::yes, VoteBehavior::Default(Yes|No)", | |
| features_contents, | |
| re.MULTILINE, | |
| ) | |
| fix_hits = re.findall( | |
| r"^ *XRPL_FIX *\(([a-zA-Z0-9_]+), * Supported::yes,", | |
| features_contents, | |
| re.MULTILINE, | |
| ) | |
| all_supported_amendments = feature_hits + ["fix" + f for f in fix_hits] | |
| return all_supported_amendments | |
| else: | |
| print(f"Failed to fetch file. Status code: {response.status_code}") | |
| return [] | |
| def fetch_rippled_amendments(): | |
| # Send a GET request | |
| try: | |
| response = requests.get(RIPPLED_FEATURES_FILE, timeout=30) # 30 second timeout | |
| # Check for successful request | |
| if response.status_code == 200: | |
| features_contents = response.text | |
| feature_hits = re.findall( | |
| r"^ *XRPL_FEATURE *\(([a-zA-Z0-9_]+), * Supported::yes, VoteBehavior::Default(Yes|No)", | |
| features_contents, | |
| re.MULTILINE, | |
| ) | |
| fix_hits = re.findall( | |
| r"^ *XRPL_FIX *\(([a-zA-Z0-9_]+), * Supported::yes,", | |
| features_contents, | |
| re.MULTILINE, | |
| ) | |
| all_supported_amendments = feature_hits + ["fix" + f for f in fix_hits] | |
| return all_supported_amendments | |
| else: | |
| print(f"Failed to fetch file. Status code: {response.status_code}") | |
| return [] | |
| except requests.exceptions.RequestException as e: | |
| print(f"Error making request: {e}") | |
| return [] |
🧰 Tools
🪛 Ruff (0.8.2)
37-37: SyntaxError: Expected ',', found name
43-43: SyntaxError: Expected ',', found name
43-44: SyntaxError: Expected ')', found newline
🪛 GitHub Actions: Integration test
[error] 37-37: SyntaxError: invalid syntax
There was a problem hiding this comment.
Can you give me examples of errors (or) circumstances where this try-catch block is helpful? I'd prefer to keep the code concise rather than catch all possible errors.
Certain errors are acceptable in this case. It is not used in production. Developers can look at the logs of the CI/CD pipeline, in case of any errors.
There was a problem hiding this comment.
@ckeshava You're right about keeping CI/CD code concise. The try-catch block would primarily help with network-related issues like:
- Temporary GitHub outages or rate limiting
- Network connectivity problems
- Request timeouts
- DNS resolution failures
Without exception handling, these would cause the script to crash with a potentially confusing traceback rather than a clear error message. However, since this is for CI/CD only and developers can check the logs, letting it fail with the default Python traceback is a reasonable approach if you prefer concise code.
I'll defer to your judgment on this one, as your preference for simplicity makes sense in this context.
✏️ Learnings added
Learnt from: ckeshava
PR: XRPLF/xrpl-py#824
File: tools/fetch_rippled_amendments.py:0-0
Timestamp: 2025-04-07T23:32:27.045Z
Learning: For CI/CD scripts in the xrpl-py repository, ckeshava prefers concise code over comprehensive error handling since developers can check the logs for error details.
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.
| path=/var/lib/rippled/db/nudb | ||
| advisory_delete=0 | ||
|
|
||
| # How many ledgers do we want to keep (history)? |
There was a problem hiding this comment.
Why are all the comments removed?
There was a problem hiding this comment.
I've explained in point-2 under "Context of Change": #824 (comment)
There was a problem hiding this comment.
The comments are for us, not for rippled - they're supposed to be ignored by rippled
|
|
||
| import requests | ||
|
|
||
| CONFIG_FILE = os.path.join(os.getcwd(), ".ci-config", "rippled.cfg") |
There was a problem hiding this comment.
It would be much more efficient to use the feature RPC on Devnet
There was a problem hiding this comment.
There are cases where new rippled amendments have been merged into the cpp code, but haven't been deployed into a devnet yet. I suspect this happens frequently for new amendments. Parsing the develop branch of the rippled repository covers this case.
I'm not strongly opposed to using the feature command on the devnet.
| run: | | ||
| poetry run python tools/fetch_rippled_amendments.py | ||
|
|
||
| - name: Add and commit rippled.cfg |
There was a problem hiding this comment.
Why run this as a part of integration tests rather than have a separate PR open on a cron job to update it? Feels potentially confusing to anyone submitting a PR.
There was a problem hiding this comment.
Integration tests make use of a docker container. The rippled.cfg file is used to kick-start the rippled server with custom amendments. The results of the integration tests differ based on which amendments have been enabled in the standalone mode.
separate PR open on a cron job to update it
Are you suggesting Github Actions cron jobs (or) running a cron-job from my personal machine to periodically fetch rippled PRs ?
There was a problem hiding this comment.
A GitHub Action cron job.
|
Yes, I agree with you. But we already have a rippled-example.cfg file in
the rippled repository, which is far-more comprehensive that the version in
the xrpl-py repository.
Secondly, I don't know of good ConfigParser libraries in Python which
preserve the comments. The alternatives I found have not been maintained
for the last few years ://
Regards,
Keshava
…On Thu, Apr 10, 2025 at 1:07 PM Mayukha Vadari ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In .ci-config/rippled.cfg
<https://urldefense.com/v3/__https://github.com/XRPLF/xrpl-py/pull/824*discussion_r2038232856__;Iw!!PZTMFYE!9PQuLNIKchu-Ad16dOdAfHasypavGFglO4XKu2vmf4-QfsELdTkcpnkelKThSMoPp3AD5lW0EvFKFMGJyot-90OI3w$>
:
> [node_db]
-type=NuDB
-path=/var/lib/rippled/db/nudb
-advisory_delete=0
-
-# How many ledgers do we want to keep (history)?
The comments are for us, not for rippled - they're *supposed* to be
ignored by rippled
—
Reply to this email directly, view it on GitHub
<https://urldefense.com/v3/__https://github.com/XRPLF/xrpl-py/pull/824*discussion_r2038232856__;Iw!!PZTMFYE!9PQuLNIKchu-Ad16dOdAfHasypavGFglO4XKu2vmf4-QfsELdTkcpnkelKThSMoPp3AD5lW0EvFKFMGJyot-90OI3w$>,
or unsubscribe
<https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/AFB4TNMOEIDHOYKHXMQWIDL2Y3FOVAVCNFSM6AAAAAB2UW6IKWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDONJYGIZTINZYGE__;!!PZTMFYE!9PQuLNIKchu-Ad16dOdAfHasypavGFglO4XKu2vmf4-QfsELdTkcpnkelKThSMoPp3AD5lW0EvFKFMGJyotuIZynGA$>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Why do you need the library? Why not just append text to the end of the file? |
High Level Overview of Change
This PR aims to bring parity between new rippled amendments and the ones enabled in the
xrpl-pyintegration tests. Developers often forget to include new amendments. This could result in behavioral differences between testing the client libraries and usage in real-world production environments.The PR defines a new Python script to fetch amendments. A new Github Actions step is defined for committing an updated rippled.cfg file.
This update is triggered on every pull-request triggered from a branch residing in this remote: https://github.com/XRPLF/xrpl-py (i.e. no external forks). This step requires write-permissions into the said branch.
Context of Change
Note:
developbranch ofrippledrepository is monitored for new amendments.ConfigParserlibrary, comments inside the configuration file are not preserved. While this is not ideal behavior, I'm open to hearing alternative library suggestions. All the comments in the cfg file are non-essential. They have already been covered elsewhere in the XRPL documentation.Type of Change
Did you update CHANGELOG.md?
Test Plan