Skip to content

fix: custom preset cached on target's preset.yml#270

Merged
fboucquez merged 4 commits intodevfrom
custom_preset_cache
Nov 24, 2021
Merged

fix: custom preset cached on target's preset.yml#270
fboucquez merged 4 commits intodevfrom
custom_preset_cache

Conversation

@fboucquez
Copy link
Copy Markdown
Owner

@fboucquez fboucquez commented Jun 21, 2021

This is a small improvement. The original custom preset (without any private key) is cached in the preset.yml of the target folder. If the user upgrading doesn't provide the --customPreset, that cache is used. Currently, if the user is not providing a custom preset when --upgrading, a custom value (for example maxUnlockedAccounts: 20) could be reset to its bootstrap's default (10).

--preset and --assembly are already derived from the target folder when not provided.

How to upgrade a node before:

symbol-bootstrap start --upgrade --preset mainnet --assembly dual --customPreset my-custom-preset.yml

How to upgrade a node after:

symbol-bootstrap start --upgrade

The custom preset is loaded from the cache inside the target folder, the same applies to the preset and assembly values.
The user can still provide a custom preset if he wants to change a property for example.

Note: User needs to run the full --upgrade command one more time so the custom preset cache is populated.

Another fix: The --preset bootstrap is not the default anymore. Most people are creating nodes for testnet and mainnet, not for local testing networks like it was originally intended. The preset must be explicitly defined when starting a new node.

@fboucquez fboucquez requested review from Wayonb and segfaultxavi and removed request for segfaultxavi June 21, 2021 11:48
@fboucquez fboucquez changed the title fix: custom preset cached on target fix: custom preset cached on target's preset.yml Jun 21, 2021
@mm-s
Copy link
Copy Markdown

mm-s commented Jun 21, 2021

How does it behave when the custom preset contains private keys ?

@fboucquez fboucquez force-pushed the custom_preset_cache branch from 33d3c05 to 8b02b2d Compare June 21, 2021 12:09
@fboucquez
Copy link
Copy Markdown
Owner Author

How does it behave when the custom preset contains private keys?

Private keys are not stored in the ./target/preset.yml file, this files contains the catched custom preset.

You can create your custom preset in your local offline machine, generate the configuration, zip the target folder, and send it to your node without the node having the keys at all. With this PR, you don't need to send the custom preset either.

Copy link
Copy Markdown
Contributor

@segfaultxavi segfaultxavi left a comment

Choose a reason for hiding this comment

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

I understand the purpose of this PR and I like it very much. But I do not understand that sentence in the README.

Comment thread CHANGELOG.md Outdated
Comment thread src/service/ConfigLoader.ts Outdated
oldPresetData?.preset ||
Preset.bootstrap;

const preset = params.preset || params.customPresetObject?.preset || customPresetFileObject?.preset || oldPresetData?.preset;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Whats the order of precedence for the presets? Should customPresetFileObject comes before customPresetObject?

Copy link
Copy Markdown
Owner Author

@fboucquez fboucquez Jun 23, 2021

Choose a reason for hiding this comment

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

if the user is providing both, a file and a custom preset object, the custom preset object is priority. This is the case when you use bootstrap as a library.

I would probably need to revisit this, especially in private networks when the network.yml is provided instead of built-in bootstrap.

I have added further unit testing around this.

@segfaultxavi segfaultxavi self-requested a review June 22, 2021 13:50
segfaultxavi
segfaultxavi previously approved these changes Jun 22, 2021
@fboucquez fboucquez force-pushed the custom_preset_cache branch 2 times, most recently from 67327f7 to c527a1d Compare June 23, 2021 12:12
@fboucquez
Copy link
Copy Markdown
Owner Author

Superseded by #277

@fboucquez fboucquez closed this Jul 6, 2021
@fboucquez fboucquez deleted the custom_preset_cache branch July 6, 2021 13:46
@fboucquez fboucquez restored the custom_preset_cache branch July 21, 2021 13:28
@fboucquez fboucquez reopened this Jul 21, 2021
@fboucquez fboucquez force-pushed the custom_preset_cache branch from c527a1d to 6fa9fc8 Compare July 22, 2021 10:14
@fboucquez fboucquez requested a review from yilmazbahadir July 22, 2021 10:20
@fboucquez fboucquez force-pushed the custom_preset_cache branch 3 times, most recently from 3400a62 to 90b972b Compare August 3, 2021 01:19
@fboucquez fboucquez requested a review from Wayonb August 13, 2021 11:44
@fboucquez
Copy link
Copy Markdown
Owner Author

Hi @Wayonb I'm keen to merge this one. Is it ok for you?

@sonarqubecloud
Copy link
Copy Markdown

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@sonarqubecloud
Copy link
Copy Markdown

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@fboucquez fboucquez force-pushed the custom_preset_cache branch 2 times, most recently from 1f02207 to 7758bd8 Compare October 30, 2021 21:55
fix: --preset bootstrap is not the default anymore.
@fboucquez fboucquez requested review from Jaguar0625 and removed request for yilmazbahadir November 22, 2021 19:57
Comment thread docs/config.md Outdated
OPTIONS
-a, --assembly=assembly The assembly, example "dual" for testnet. If not provided, the value is
resolved from the target/preset.yml file.
-a, --assembly=assembly The assembly that define the node(s) layout. It can be provided via custom
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

that define*s

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

all three docs files.

as there is a lot of similarity, i was wondering: are you generating these manually or automatically?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Fixed. They are automatically generated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

if you don't already, consider adding a build section to the readme (if you don't already) about how to generate these files and the expected test reporting files too. can be separate PR.

Comment thread docs/start.md Outdated
Comment thread README.md Outdated
Comment thread src/commands/pack.ts Outdated
Comment thread src/commands/start.ts Outdated
Comment thread src/commands/updateVotingKeys.ts Outdated
Comment thread src/service/ConfigLoader.ts
Comment thread test/service/ConfigLoader.test.ts
@fboucquez fboucquez requested a review from Jaguar0625 November 24, 2021 00:18
Jaguar0625
Jaguar0625 previously approved these changes Nov 24, 2021
Fernando added 2 commits November 24, 2021 18:05
# Conflicts:
#	CHANGELOG.md
#	src/commands/config.ts
#	src/commands/pack.ts
#	src/commands/start.ts
#	src/commands/updateVotingKeys.ts
#	src/commands/wizard.ts
#	src/service/ConfigLoader.ts
#	test/service/ConfigService.test.ts
@sonarqubecloud
Copy link
Copy Markdown

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@fboucquez fboucquez merged commit 420e2c1 into dev Nov 24, 2021
@fboucquez fboucquez deleted the custom_preset_cache branch November 24, 2021 22:12
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.

5 participants