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

[Infra] Add app/servicer keys to LocalNet genesis #781

Merged
merged 2 commits into from
Jun 1, 2023

Conversation

adshmh
Copy link
Contributor

@adshmh adshmh commented May 23, 2023

Description

Summary generated by Reviewpad on 01 Jun 23 18:05 UTC

This pull request adds 2 applications and 2 servicers to the genesis.json file for LocalNet configuration, which can be used for testing the CLI. It is part of work on issue #754. The patch includes changes to the CHANGELOG.md and README.md files to reflect the added addresses. The relevant options for this pull request are "Documentation" and "Other," specifically "added 2 applications and 2 servicers to genesis.json for LocalNet."

Issue

Part of work on #754

Type of change

Please mark the relevant option(s):

  • New feature, functionality or library
  • Bug fix
  • Code health or cleanup
  • Major breaking change
  • Documentation
  • Other Added 2 applications and 2 servicers to genesis.json for LocalNet

List of changes

  • Added 2 applications to genesis.json for LocalNet
  • Added 2 servicers to genesis.json for LocalNet

Testing

  • make develop_test; if any code changes were made
  • make test_e2e on k8s LocalNet; if any code changes were made
  • e2e-devnet-test passes tests on DevNet; if any code was changed
  • Docker Compose LocalNet; if any major functionality was changed or introduced
  • k8s LocalNet; if any infrastructure or configuration changes were made

Required Checklist

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added, or updated, godoc format comments on touched members (see: tip.golang.org/doc/comment)
  • I have tested my changes using the available tooling
  • I have updated the corresponding CHANGELOG

If Applicable Checklist

  • I have updated the corresponding README(s); local and/or global
  • I have added tests that prove my fix is effective or that my feature works
  • I have added, or updated, mermaid.js diagrams in the corresponding README(s)
  • I have added, or updated, documentation and mermaid.js diagrams in shared/docs/* if I updated shared/*README(s)

@reviewpad reviewpad bot added small Pull request is small waiting-for-review labels May 23, 2023
@adshmh adshmh added the utility Utility specific changes label May 23, 2023
@adshmh adshmh self-assigned this May 23, 2023
Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

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

A couple small NITs but otherwise LGTM. Once CI passes, the pattern we use is "Squash & Merge" and copy-paste the PR description into the commit message.

@@ -148,6 +148,21 @@ For example:
- `0010297b55fc9278e4be4f1bcfe52bf9bd0443f8` is a servicer #001.
- `314019dbb7faf8390c1f0cf4976ef1215c90b7e4` is an application #314.


#### Applications staked on LocalNet
Applications with the following addresses are staked on LocalNet, through [applications field of the genesis.json in the configuration](https://github.com/pokt-network/pocket/blob/main/build/localnet/manifests/configs.yaml#L4088)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Applications with the following addresses are staked on LocalNet, through [applications field of the genesis.json in the configuration](https://github.com/pokt-network/pocket/blob/main/build/localnet/manifests/configs.yaml#L4088)
Applications with the following addresses are staked on LocalNet, through the [applications field of the genesis.json in the LocalNet configuration](https://github.com/pokt-network/pocket/blob/main/build/localnet/manifests/configs.yaml#L4088)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the review. Fixed.

These addresses can be used for e.g. testing the CLI.

#### Servicers staked on LocalNet
Servicers with the following addresses are staked on LocalNet, through [servicers field of the genesis.json in the configuration](https://github.com/pokt-network/pocket/blob/main/build/localnet/manifests/configs.yaml#L4120)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Servicers with the following addresses are staked on LocalNet, through [servicers field of the genesis.json in the configuration](https://github.com/pokt-network/pocket/blob/main/build/localnet/manifests/configs.yaml#L4120)
Servicers with the following addresses are staked on LocalNet, through the [servicers field of the genesis.json in the LocalNet configuration](https://github.com/pokt-network/pocket/blob/main/build/localnet/manifests/configs.yaml#L4120)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the review. Fixed.

@adshmh adshmh force-pushed the fix-add-applications-services-to-localnet-genesis branch 2 times, most recently from 2db33ab to d3d224b Compare May 24, 2023 00:19
@adshmh
Copy link
Contributor Author

adshmh commented May 24, 2023

A couple small NITs but otherwise LGTM. Once CI passes, the pattern we use is "Squash & Merge" and copy-paste the PR description into the commit message.

Thank you for the review. Updated the commit message.

This pull request adds two applications and two servicers to the
genesis.json in LocalNet configuration. The new addresses can be used
for testing the CLI.

Part of work on #754

Please mark the relevant option(s):

- [ ] New feature, functionality or library
- [ ] Bug fix
- [ ] Code health or cleanup
- [ ] Major breaking change
- [X] Documentation
- [x] Other Added 2 applications and 2 servicers to genesis.json for
  LocalNet

- Added 2 applications to genesis.json for LocalNet
- Added 2 servicers to genesis.json for LocalNet

- [ ] `make develop_test`; if any code changes were made
- [ ] `make test_e2e` on [k8s
  LocalNet](https://github.com/pokt-network/pocket/blob/main/build/localnet/README.md);
if any code changes were made
- [ ] `e2e-devnet-test` passes tests on
  [DevNet](https://pocketnetwork.notion.site/How-to-DevNet-ff1598f27efe44c09f34e2aa0051f0dd);
if any code was changed
- [ ] [Docker Compose
  LocalNet](https://github.com/pokt-network/pocket/blob/main/docs/development/README.md);
if any major functionality was changed or introduced
- [X] [k8s
  LocalNet](https://github.com/pokt-network/pocket/blob/main/build/localnet/README.md);
if any infrastructure or configuration changes were made

- [ ] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added, or updated, [`godoc` format
  comments](https://go.dev/blog/godoc) on touched members (see:
[tip.golang.org/doc/comment](https://tip.golang.org/doc/comment))
- [x] I have tested my changes using the available tooling
- [ ] I have updated the corresponding CHANGELOG

- [x] I have updated the corresponding README(s); local and/or global
- [ ] I have added tests that prove my fix is effective or that my
  feature works
- [ ] I have added, or updated,
  [mermaid.js](https://mermaid-js.github.io) diagrams in the
corresponding README(s)
- [ ] I have added, or updated, documentation and
  [mermaid.js](https://mermaid-js.github.io) diagrams in `shared/docs/*`
if I updated `shared/*`README(s)

Signed-off-by: Arash Deshmeh <[email protected]>
@adshmh adshmh force-pushed the fix-add-applications-services-to-localnet-genesis branch from d3d224b to b5db741 Compare May 24, 2023 00:21
Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

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

@adshmh I tried to review the changes since my last review and I'm guessing you git fooed to clean up some of your local commits.

It really helps the reviewer to see what changes took place between each review if we don't modify the git history, especially for PRs that are larger and take longer.

Per our review guidelines (#773), we always do a "squash and merge" when we merge the PR into main, so the history will be clean on main regardless.

Screenshot 2023-05-23 at 8 52 00 PM
Screenshot 2023-05-23 at 8 51 56 PM

@adshmh
Copy link
Contributor Author

adshmh commented May 24, 2023

I tried to review the changes since my last review and I'm guessing you git fooed to clean up some of your local commits.

Sorry about that, wasn't local commits, did a push -f by mistake (the CI's timezone does not match mine, so CHANGELOG checks failed on CI after having passed locally).

@Olshansk
Copy link
Member

I tried to review the changes since my last review and I'm guessing you git fooed to clean up some of your local commits.

Sorry about that, wasn't local commits, did a push -f by mistake (the CI's timezone does not match mine, so CHANGELOG checks failed on CI after having passed locally).

No worries. Regardless, see my previous comment here about merging it in.

Seems like you might need to merge with master first since there are merge conflicts since this was last reviewed.

@Olshansk
Copy link
Member

Olshansk commented Jun 1, 2023

@adshmh Did you still want to merge this in? If so, it's approved but you'll need to merge with main and resolve conflicts first. I also believe you have the proper github permissions to do so.

@Olshansk
Copy link
Member

Olshansk commented Jun 1, 2023

@adshmh Following up on an offline discussion around merge conflicts related to changelogs.

I took the liberty of resolving the conflict (and pushing the change) and recorded a video as a reference that we can have publically on the steps taken. Let me know if you have any follow up questions on this.

Screen.Recording.2023-06-01.at.11.04.36.AM.mov

@Olshansk
Copy link
Member

Olshansk commented Jun 1, 2023

Per our review guidelines (#773), we always do a "squash and merge" when we merge the PR into main, so the history will be clean on main regardless.

@adshmh Seems like everything is green so feel free to merge if you think it's ready!

@adshmh adshmh merged commit 7632076 into main Jun 1, 2023
@adshmh
Copy link
Contributor Author

adshmh commented Jun 1, 2023

Per our review guidelines (#773), we always do a "squash and merge" when we merge the PR into main, so the history will be clean on main regardless.

@adshmh Seems like everything is green so feel free to merge if you think it's ready!

Done. Thanks!

@Olshansk Olshansk deleted the fix-add-applications-services-to-localnet-genesis branch June 2, 2023 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
small Pull request is small utility Utility specific changes waiting-for-review
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants