Skip to content
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

ci: Add a script to generate changes to ci-config #2431

Merged
merged 13 commits into from
Aug 18, 2023
Merged

Conversation

JST5000
Copy link
Contributor

@JST5000 JST5000 commented Aug 9, 2023

High Level Overview of Change

Automate figuring out which amendments to enable in our CI docker container.

Context of Change

Periodically we need to update the rippled.cfg we use in the standalone node we use during CI so that the library is being tested against the latest amendments. This adds a script to get the hashes of any new amendments so it's easy to update our CI without a bunch of manual lookups / comparisons.

Type of Change

  • New feature (non-breaking change which adds functionality)
  • Documentation Updates

Test Plan

Manually tested the script against various networks / removing entries from the config file to see if it noticed.

Future Tasks

Automatically generate a PR to update the CI file directly with hashes + names instead of requiring a manual lookup on the known amendments page.

.ci-config/rippled.cfg Outdated Show resolved Hide resolved
# Added August 9th, 2023
86E83A7D2ECE3AD5FA87AB2195AE015C950469ABF0B72EAACED318F74886AE90 CryptoConditionsSuite
AE35ABDEFBDE520372B31C957020B34A7A4A9DC3115A69803A44016477C84D6E fixNFTokenRemint
B2A4DB846F0891BF2C76AB2F2ACC8F5B4EC64437135C6E56F3F859DE5FFD5856 ExpandedSignerList
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably be added above since it was much older. Added in 1.9.1.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might need to add an integration test for WalletLocator and the size of SignerLists

Copy link
Contributor Author

@JST5000 JST5000 Aug 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be alright to leave that for a separate PR? (I'm not exactly sure what we'd be testing for that - maybe you can flesh out an Issue to add it w/ an idea for what we'd want to test on the client library side?)

.ci-config/rippled.cfg Outdated Show resolved Hide resolved
.ci-config/getNewAmendments.js Show resolved Hide resolved
@@ -2,6 +2,9 @@

## Unreleased

### Added
* Updated to include latest updates to `definitions.json` including changes for the `fixNFTokenRemint` amendment.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: note that this includes AMM definitions updates too.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the AMM definitions wait for the AMM PR instead of being done here?

Copy link
Contributor Author

@JST5000 JST5000 Aug 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to just match rippled's develop as best as possible instead of manually editing the definitions.json file after running the script. Plus, I don't think there's any harm in having more definitions available.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we hold off on this PR until that's merged in, then? Since that one's going to be merged soon anyways.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was generated from develop already - not sure why the definitions were there before the big AMM PR, but it seems they're already defined.

Copy link
Collaborator

@mvadari mvadari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this automatically be run in a Github Action, say once a week?

@mvadari
Copy link
Collaborator

mvadari commented Aug 17, 2023

super nit: could the PR title avoid the word "automate"? That's part of the reason why I asked about a GitHub Action - I was expecting something that automatically ran the script.

@JST5000 JST5000 changed the title ci: Automate generating changes to ci-config ci: Add a script to generate changes to ci-config Aug 17, 2023
@JST5000
Copy link
Contributor Author

JST5000 commented Aug 17, 2023

super nit: could the PR title avoid the word "automate"? That's part of the reason why I asked about a GitHub Action - I was expecting something that automatically ran the script.

I think I wanted to automate it originally, but didn't see an easy way - I think your question was fair, I just didn't find an answer just yet and might look into it more as a follow up task. Updated the title for now since I think the changes are helpful as is.

@JST5000 JST5000 merged commit 49447a9 into main Aug 18, 2023
17 checks passed
@JST5000 JST5000 deleted the automate-config branch August 18, 2023 22:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants