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

Feat/local devnet #9

Merged
merged 5 commits into from
Aug 16, 2023
Merged

Feat/local devnet #9

merged 5 commits into from
Aug 16, 2023

Conversation

omidasadpour
Copy link
Contributor

@omidasadpour omidasadpour commented Jul 12, 2023

/close #8
/close #14
This PR will deploy rollups nodes along with anvil local node.

@omidasadpour omidasadpour self-assigned this Jul 12, 2023
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.

Let's make it simple and keep only anvil.

charts/rollups-validator-node/ci/test-values.yaml.tpl Outdated Show resolved Hide resolved
charts/rollups-validator-node/templates/_helpers.tpl Outdated Show resolved Hide resolved
charts/rollups-validator-node/templates/configmap.yaml Outdated Show resolved Hide resolved
charts/rollups-validator-node/templates/deployment.yaml Outdated Show resolved Hide resolved
charts/rollups-validator-node/ci/test-values.yaml.tpl Outdated Show resolved Hide resolved
charts/rollups-validator-node/values.yaml Show resolved Hide resolved
charts/rollups-validator-node/values.yaml Outdated Show resolved Hide resolved
charts/rollups-validator-node/templates/_helpers.tpl Outdated Show resolved Hide resolved
charts/rollups-validator-node/templates/deployment.yaml Outdated Show resolved Hide resolved
charts/rollups-validator-node/values.yaml Outdated Show resolved Hide resolved
charts/rollups-validator-node/templates/_helpers.tpl Outdated Show resolved Hide resolved
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.

asked for some changes on my last review comments

@omidasadpour omidasadpour changed the base branch from main to release/0.5.1 July 18, 2023 13:24
@endersonmaia
Copy link
Collaborator

squash the fixups

@omidasadpour
Copy link
Contributor Author

omidasadpour commented Jul 19, 2023

/close #14 We can use the command below to test the graphql-server connectivity after install the release.

helm test $RELEASE_NAME

In CI, the chart-testing section will test the chart automatically.

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.

The only thing missing is the labels: empty line, I've left a suggestion on how to solve it.

@omidasadpour omidasadpour modified the milestones: v0.5.1, v0.5.2 Aug 2, 2023
Base automatically changed from release/0.5.1 to main August 3, 2023 12:06
@endersonmaia endersonmaia modified the milestones: v0.5.2, v1.0.0 Aug 8, 2023
@omidasadpour
Copy link
Contributor Author

The only thing missing is the labels: empty line, I've left a suggestion on how to solve it.

Fixed the problem !

@omidasadpour omidasadpour changed the base branch from main to release/1.0.0 August 11, 2023 08:20
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.

So that this PR entres the next release, you should target the branch feature/rollups-1.0.0.

There, I already split the deployment.yaml in 3 different files.

charts/rollups-validator-node/templates/deployment.yaml Outdated Show resolved Hide resolved
@omidasadpour
Copy link
Contributor Author

omidasadpour commented Aug 11, 2023

So that this PR entres the next release, you should target the branch feature/rollups-1.0.0.

There, I already split the deployment.yaml in 3 different files.

This PR is older than the feature/rollups-1.0.0. and is ready to merge. also the feature/rollups-1.0.0 PR is still in Draft.

I think it would be better if we close this PR and then sync the feature/rollups-1.0.0 with the latest changes of the release/1.0.0 branch .

What do U think ?

@endersonmaia
Copy link
Collaborator

So that this PR entres the next release, you should target the branch feature/rollups-1.0.0.
There, I already split the deployment.yaml in 3 different files.

This PR is older than the feature/rollups-1.0.0. and is ready to merge. also the feature/rollups-1.0.0 PR is still in Draft.

I think it would be better if we close this PR and then sync the feature/rollups-1.0.0 with the latest changes of the release/1.0.0 branch .

What do U think ?

Whatever is easy for you.

We decided that devnet should go into release 1.0.0, even if it's still a draft, we'll only release 1.0.0 when everything is working.

@omidasadpour omidasadpour changed the base branch from release/1.0.0 to next August 14, 2023 05:49
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.

Need to squash the fixups and remote DELETE commits.

The version/revision will be tackled on a release commit, let's keep the commit into next branch generic and without version information

Maybe this next branch strategy won't work here because of the way ct makes releases and checks for changes between branches, IDK.

I'll assume that we can change the next history before a final merge into main.

charts/rollups-validator-node/README.md Outdated Show resolved Hide resolved
@omidasadpour omidasadpour force-pushed the feat/local-devnet branch 2 times, most recently from 534c72a to 7d13f6c Compare August 14, 2023 17:25
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.

the last commit comment is DOC: when it should be doc:, lowercase.

charts/rollups-validator-node/templates/deployment.yaml Outdated Show resolved Hide resolved
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.

I think we could reduce the depth for the validator.localnode.anvil removing the .anvil element.

validator.localnode should be enough, and the use of anvil would be an "implementation detail¨, so that if the user changes valiator.localnode. parameters, they can change to hardhat or whatever, for ex., and adapt where is needed.

@omidasadpour
Copy link
Contributor Author

I think we could reduce the depth for the validator.localnode.anvil removing the .anvil element.

validator.localnode should be enough, and the use of anvil would be an "implementation detail¨, so that if the user changes valiator.localnode. parameters, they can change to hardhat or whatever, for ex., and adapt where is needed.

I think it's not a good solution. because we have lots of dependencies like initcontainers based on anvil.

So, we may have other localnodes in future like hardhat.

I think we should not change the structure.

@omidasadpour omidasadpour merged commit 6e7bff1 into next Aug 16, 2023
3 of 4 checks passed
@omidasadpour omidasadpour deleted the feat/local-devnet branch August 16, 2023 11:52
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.

Provide local devnet support
2 participants