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

cardano-testnet-test: small improvements #5962

Merged
merged 2 commits into from
Sep 4, 2024

Conversation

smelc
Copy link
Contributor

@smelc smelc commented Aug 28, 2024

Description

This PR removes initialization of fields in cardano-testnet-test that were useless because they were initializing said fields with the same value as the default values. It also does QoL improvements.

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • Self-reviewed the diff

@smelc smelc changed the title cardano-testnet-test: small simplification cardano-testnet-test: remove (unused) ability to pass NodeConfigurationYaml to individual noes Aug 28, 2024
@smelc smelc changed the title cardano-testnet-test: remove (unused) ability to pass NodeConfigurationYaml to individual noes cardano-testnet-test: remove (unused) ability to pass NodeConfigurationYaml to individual nodes Aug 28, 2024
@smelc smelc marked this pull request as ready for review August 28, 2024 11:41
@smelc smelc requested a review from a team as a code owner August 28, 2024 11:41
@smelc smelc force-pushed the smelc/cardano-testnet-remove-unused-option branch from 1753af9 to 0d6082e Compare August 28, 2024 12:32
Base automatically changed from smelc/honor-cardano-testnet-max-supply-value to master August 29, 2024 18:25
@smelc smelc force-pushed the smelc/cardano-testnet-remove-unused-option branch 3 times, most recently from 80fb3ee to 81c84f7 Compare September 3, 2024 14:41
@smelc smelc changed the title cardano-testnet-test: remove (unused) ability to pass NodeConfigurationYaml to individual nodes cardano-testnet-test: remove (unused) ability to specify P2P flag + small improvements Sep 3, 2024
@smelc smelc force-pushed the smelc/cardano-testnet-remove-unused-option branch from 81c84f7 to a245cd5 Compare September 4, 2024 07:46
@coot
Copy link
Contributor

coot commented Sep 4, 2024

Are we using P2P stack in tests? Note that the non-p2p stack will be removed at some point.

@smelc
Copy link
Contributor Author

smelc commented Sep 4, 2024

@coot> I believe we don't use P2p in cardano-testnet, because here is what we have by default in the configuration.yaml used to start nodes:

→ cat /tmp/nix-shell.3H7AVw/cli-queries-test-23d51bd100cd79ee/configuration.yaml  | grep P2P
    "EnableP2P": false,

@smelc
Copy link
Contributor Author

smelc commented Sep 4, 2024

Let me amend the PR then, to keep the flag then and honor it instead of doing nothing with it 🙂

@smelc smelc marked this pull request as draft September 4, 2024 08:07
@smelc
Copy link
Contributor Author

smelc commented Sep 4, 2024

Honoring P2P will affect other work I have stacked on this one, so I'll just keep the "small improvements" bits of this PR and merge. P2P honoring will come later.

@smelc smelc force-pushed the smelc/cardano-testnet-remove-unused-option branch from a245cd5 to 47cc7cc Compare September 4, 2024 09:48
@smelc smelc changed the title cardano-testnet-test: remove (unused) ability to specify P2P flag + small improvements cardano-testnet-test: small improvements Sep 4, 2024
@smelc smelc marked this pull request as ready for review September 4, 2024 09:49
@smelc smelc enabled auto-merge September 4, 2024 09:49
@coot
Copy link
Contributor

coot commented Sep 4, 2024

Note that non-p2p will be removed.

@carbolymer carbolymer merged commit d113bdc into master Sep 4, 2024
15 of 16 checks passed
@carbolymer carbolymer deleted the smelc/cardano-testnet-remove-unused-option branch September 4, 2024 13:50
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.

3 participants