-
Notifications
You must be signed in to change notification settings - Fork 115
Update rippled.cfg file #824
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
base: main
Are you sure you want to change the base?
Changes from 6 commits
bff1aa9
4469f7b
0b0f8a0
82693e6
a53ed6c
cc6b797
4236ca6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,9 @@ jobs: | |
steps: | ||
- name: Checkout code | ||
uses: actions/checkout@v4 | ||
with: | ||
repository: ${{ github.event.pull_request.head.repo.full_name }} | ||
ref: ${{ github.event.pull_request.head.ref }} | ||
|
||
- name: Load cached .local | ||
id: cache-poetry | ||
|
@@ -30,10 +33,6 @@ jobs: | |
path: /home/runner/.local | ||
key: dotlocal-${{ env.POETRY_VERSION }} | ||
|
||
- name: Run docker in background | ||
run: | | ||
docker run --detach --rm -p 5005:5005 -p 6006:6006 --volume "${{ github.workspace }}/.ci-config/":"/etc/opt/ripple/" --name rippled-service --health-cmd="rippled server_info || exit 1" --health-interval=5s --health-retries=10 --health-timeout=2s --env GITHUB_ACTIONS=true --env CI=true --entrypoint bash ${{ env.RIPPLED_DOCKER_IMAGE }} -c "rippled -a" | ||
|
||
- name: Install poetry | ||
if: steps.cache-poetry.outputs.cache-hit != 'true' | ||
run: | | ||
|
@@ -53,6 +52,26 @@ jobs: | |
- name: Install poetry dependencies | ||
run: poetry install | ||
|
||
- name: Fetch latest rippled amendments | ||
if: ${{ github.event_name == 'pull_request' }} | ||
run: | | ||
poetry run python tools/fetch_rippled_amendments.py | ||
|
||
- name: Add and commit rippled.cfg | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Integration tests make use of a docker container. The
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A GitHub Action cron job. |
||
if: ${{ github.event_name == 'pull_request' }} | ||
uses: EndBug/add-and-commit@v9 | ||
with: | ||
message: 'Update rippled.cfg with latest amendments' | ||
author_name: GitHub Actions | ||
author_email: [email protected] | ||
committer_name: GitHub Actions | ||
committer_email: [email protected] | ||
add: .ci-config/rippled.cfg | ||
|
||
- name: Run docker in background | ||
run: | | ||
docker run --detach --rm -p 5005:5005 -p 6006:6006 --volume "${{ github.workspace }}/.ci-config/":"/etc/opt/ripple/" --name rippled-service --health-cmd="rippled server_info || exit 1" --health-interval=5s --health-retries=10 --health-timeout=2s --env GITHUB_ACTIONS=true --env CI=true --entrypoint bash ${{ env.RIPPLED_DOCKER_IMAGE }} -c "rippled -a" | ||
|
||
- name: Integration test | ||
run: | | ||
poetry run poe test_integration | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,73 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
This script fetches the latest amendments from (the `develop` branch of) rippled and | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
adds them to the `rippled.cfg` file. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import configparser | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import os | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import re | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import requests | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
CONFIG_FILE = os.path.join(os.getcwd(), ".ci-config", "rippled.cfg") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be much more efficient to use the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 I'm not strongly opposed to using the |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
FEATURES_SECTION = "features" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
RIPPLED_FEATURES_FILE = "https://raw.githubusercontent.com/XRPLF/rippled/develop/include/xrpl/protocol/detail/features.macro" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
config = configparser.ConfigParser( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
allow_no_value=True, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
config.optionxform = str # type: ignore | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
config.read(CONFIG_FILE) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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 | |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
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.
Why are all the comments removed?
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.
I've explained in point-2 under "Context of Change": #824 (comment)
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 comments are for us, not for rippled - they're supposed to be ignored by rippled