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

Feature/remove mnomenic network difinition for localnode #55

Merged

Conversation

omidasadpour
Copy link
Contributor

If localnode enabled Then do not need to define mnomenic & network.

@omidasadpour omidasadpour self-assigned this Sep 6, 2023
@omidasadpour omidasadpour changed the base branch from main to next September 6, 2023 11:13
charts/rollups-node/Chart.yaml Outdated Show resolved Hide resolved
@omidasadpour omidasadpour force-pushed the feature/remove-mnomenic-network-difinition-for-localnode branch from 4f1f7a5 to 6c15cdb Compare September 12, 2023 15:14
Copy link
Collaborator

@endersonmaia endersonmaia left a comment

Choose a reason for hiding this comment

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

There's a typo on the first commit message, s/mnomecis/mnemonic/.

Comment on lines 188 to 190
{{- if eq $chainID "" }}
{{- required "A valid .Values.dapp.network is required" $network }}
{{- end }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this if eq... is a no-op. Since there's no situation where $chainId == "".

But I don't know how a required function would work in that case, could you test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

your right. Unfortunately, Helm's template engine doesn't have built-in support for conditionally applying required based on the value of another variable.

I think we can remove this whole section.

Copy link
Contributor Author

@omidasadpour omidasadpour Sep 13, 2023

Choose a reason for hiding this comment

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

I have added a required section inside the dispatcher-deployment. So if the .Values.dapp.network doesn't define and the .Values.validator.localnode.enabled doesn't enabled.

The logs before:

Error: INSTALLATION FAILED: template: rollups-node-chart/templates/dispatcher-deployment.yaml:59:122: executing "rollups-node-chart/templates/dispatcher-deployment.yaml" at <"_">: invalid value; expected string

The logs after :

Error: INSTALLATION FAILED: execution error at (rollups-node-chart/templates/dispatcher-deployment.yaml:59:87): A valid .Values.dapp.network is required

charts/rollups-node/Chart.yaml Outdated Show resolved Hide resolved
@omidasadpour omidasadpour force-pushed the feature/remove-mnomenic-network-difinition-for-localnode branch from 6c15cdb to a2efc4d Compare September 13, 2023 02:45
@omidasadpour omidasadpour force-pushed the feature/remove-mnomenic-network-difinition-for-localnode branch from 9b62c4d to 583abbc Compare September 13, 2023 03:45
@endersonmaia endersonmaia force-pushed the feature/remove-mnomenic-network-difinition-for-localnode branch from 583abbc to 27c30d2 Compare September 13, 2023 09:09
@endersonmaia endersonmaia merged commit 96a4ef3 into next Sep 13, 2023
3 of 4 checks passed
@endersonmaia endersonmaia deleted the feature/remove-mnomenic-network-difinition-for-localnode branch September 13, 2023 09:10
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.

2 participants