Skip to content

[Utility] Update E2E feature path template doc#870

Merged
adshmh merged 5 commits intomainfrom
doc-contribution.md-for-utility-module
Jul 6, 2023
Merged

[Utility] Update E2E feature path template doc#870
adshmh merged 5 commits intomainfrom
doc-contribution.md-for-utility-module

Conversation

@adshmh
Copy link
Copy Markdown
Contributor

@adshmh adshmh commented Jul 3, 2023 •
edited by reviewpad bot
Loading

Description

A brief summary of the expected contribution steps to improve the PRs opened by new team members.

Summary generated by Reviewpad on 06 Jul 23 00:42 UTC

This pull request includes the following changes:

  1. In the .github/PULL_REQUEST_TEMPLATE.md file, added a comment block to list out all the changes made in the pull request.
  2. In the utility/doc/E2E_FEATURE_PATH_TEMPLATE.md file, added a link to a potential example to use in the improvement section.
  3. Updated the E2E Feature Implementation section with a TL;DR summary and steps for making the E2E test pass.
  4. Added goals for the POC (Proof of Concept) section and provided an example PR with added .feature and .go files.
  5. Added a link to Alessandro's KISS or Bryan's P2P in the MVC: Minimum Viable Change section.

Overall, this pull request includes improvements and additions to the pull request template and the E2E feature path documentation.

Issue

Minimize the number of large PRs, e.g. #789, which take longer to review and are more error-prone.

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

List of changes

  • Added a new CONTRIBUTING.md file under utility module's docs.

Testing

N/A

Required Checklist

N/A

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)

@adshmh adshmh added documentation Improvements or additions to documentation utility Utility specific changes labels Jul 3, 2023
@adshmh adshmh added this to the M3: Pocket RoS (Relay or Slash) milestone Jul 3, 2023
@adshmh adshmh self-assigned this Jul 3, 2023
@reviewpad reviewpad bot added small Pull request is small waiting-for-review docs labels Jul 3, 2023
@adshmh adshmh requested review from Olshansk and h5law July 3, 2023 19:10
Olshansk
Olshansk requested changes Jul 3, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@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 Left a few comments throughout. PTAL, because I do not think this standalone document adds value.

I'm trying to put myself in the shoes of someone who has not been part of the project before and they are just getting started and find this document. It might lead to more confusion than answers.

Perhaps the feedback of someone else on the team (or an external contributor) might help

adshmh and others added 2 commits July 3, 2023 17:16
@adshmh @Olshansk
Update utility/doc/CONTRIBUTING.md
75fb5c3 
Co-authored-by: Daniel Olshansky <olshansky@pokt.network>
@adshmh
Address review comments
d456bd5
@adshmh adshmh requested a review from Olshansk July 3, 2023 21:44
Olshansk
Olshansk requested changes Jul 3, 2023
View reviewed changes
utility/doc/E2E_FEATURE_PATH_TEMPLATE.md Outdated
- Additions / modifications to the [E2E testing framework](../../e2e)
- Additions / modifications to the documentation, code structure and diagrams

### Deliverables
Copy link
Copy Markdown
Collaborator

@Olshansk Olshansk Jul 3, 2023

Choose a reason for hiding this comment

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

Lines 92 through 111 contain the deliverables that the author of these github issues would copy paste when create a new issue. This way, it'll be directly in the actual ticket when we assign it to someone (on protocol, on backend, externally, etc...).

Do you think we should merge the two into one or keep this as a separate list of deliverables?

Copy link
Copy Markdown
Contributor Author

@adshmh adshmh Jul 4, 2023

Choose a reason for hiding this comment

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

Great point. Not sure. As you pointed out in the previous comment, the deliverables listed by this PR are possibly too specific to trustless relays work, whereas the ones on Lines 92 through 111 are very reasonable but also very generic.

Perhaps we can even keep the original deliverables, i.e. Lines 92 - 111, and use this new list and/or example PR as one way these deliverables can be fullfilled?

Copy link
Copy Markdown
Collaborator

@Olshansk Olshansk Jul 4, 2023

Choose a reason for hiding this comment

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

@adshmh I'll leave the call to you because I'm very biased by the context I have (as hard as I try not to) regarding what a new member (internal or external) of the protocol team needs. Try to think what YOU would have needed updated to make it clear and obvious

  1. I think having two Deliverables sections is not great (confusing, extra, reading, etc...)?
  2. What do you think would have benefited you more?

Copy link
Copy Markdown
Contributor Author

@adshmh adshmh Jul 4, 2023

Choose a reason for hiding this comment

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

Thank you for the clarification. I have updated the docs with what I think could help more.

Olshansk
Olshansk reviewed Jul 4, 2023
View reviewed changes
utility/doc/E2E_FEATURE_PATH_TEMPLATE.md Outdated
- Additions / modifications to the [E2E testing framework](../../e2e)
- Additions / modifications to the documentation, code structure and diagrams

### Deliverables
Copy link
Copy Markdown
Collaborator

@Olshansk Olshansk Jul 4, 2023

Choose a reason for hiding this comment

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

@adshmh I'll leave the call to you because I'm very biased by the context I have (as hard as I try not to) regarding what a new member (internal or external) of the protocol team needs. Try to think what YOU would have needed updated to make it clear and obvious

  1. I think having two Deliverables sections is not great (confusing, extra, reading, etc...)?
  2. What do you think would have benefited you more?

Olshansk
Olshansk requested changes Jul 4, 2023
View reviewed changes
utility/doc/E2E_FEATURE_PATH_TEMPLATE.md Outdated
- This will not be merged, but used to A) get a review on the approach and B) breakup the work into a list of PRs
- Should include the bare minimum E2E test(s) to test/demo the assigned feature: [a `.feature` file](https://github.com/pokt-network/pocket/pull/869/files#diff-30c6c02e8594cf72662ab975e75a810a5bbd702f274e2eaf160c97ec14f5e642) and [the updated `.go` steps file](https://github.com/pokt-network/pocket/pull/869/files#diff-01dec4121ae8acb7a1f4bb72a6c2104827d2c2d2197eb5f45fa5c032ffba32cd)

2. [The list of PRs needed for making the E2E test pass](https://github.com/pokt-network/pocket/pull/869#issuecomment-1618484939)
Copy link
Copy Markdown
Collaborator

@Olshansk Olshansk Jul 4, 2023

Choose a reason for hiding this comment

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

See line 3 of this document. Maybe we should just update that instead?

Copy link
Copy Markdown
Contributor Author

@adshmh adshmh Jul 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure I fully understand this point, updated line 3 and used this as an example. Please advise if I interpreted this incorrectly.

@adshmh adshmh requested a review from Olshansk July 4, 2023 17:15
@adshmh adshmh changed the title [Utility] Create CONTRIBUTING.md [Utility] Update E2E feature path template doc Jul 4, 2023
Olshansk
Olshansk approved these changes Jul 5, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@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.

Please consider the suggestion I made and then feel free to merge!

@adshmh @Olshansk
Update utility/doc/E2E_FEATURE_PATH_TEMPLATE.md
e207246 
Co-authored-by: Daniel Olshansky <olshansky@pokt.network>
@adshmh
Copy link
Copy Markdown
Contributor Author

adshmh commented Jul 6, 2023

Please consider the suggestion I made and then feel free to merge!

Thank you for the review. PR updated. Will merge once CI checks pass.

@adshmh adshmh merged commit fbfc4f7 into main Jul 6, 2023
bryanchriswhite added a commit that referenced this pull request Jul 7, 2023
@bryanchriswhite
Merge remote-tracking branch 'pokt/main' into feat/integrate-bg-router
66afa18 
* pokt/main:
  [Utility] Update E2E feature path template doc (#870)
  [IBC] Add nil check on proof for membership and non-membership proof creation (#877)
  Added git diff state to devlog10
  Devlog 10 (#872)
  [Documentation] Add IBC Module introduction as an example (#853)
  [Persistence][Bug] Fix Actor Schema Assignment for ValidatorActor in GetActor (#857)
  QOL: add bash completion for p1 to localnet client (#865)
bryanchriswhite added a commit that referenced this pull request Jul 7, 2023
@bryanchriswhite
Merge branch 'feat/integrate-bg-router' into chore/introduce-submodule
dd73031 
* feat/integrate-bg-router:
  fix: goimports
  fix: unstaked actor bootstrapping FSM transition
  chore: add error log
  test: improve background router validation test
  docs: fix mistake in peer discovery section
  [Utility] Update E2E feature path template doc (#870)
  [IBC] Add nil check on proof for membership and non-membership proof creation (#877)
  Added git diff state to devlog10
  Devlog 10 (#872)
  [Documentation] Add IBC Module introduction as an example (#853)
  [Persistence][Bug] Fix Actor Schema Assignment for ValidatorActor in GetActor (#857)
  QOL: add bash completion for p1 to localnet client (#865)
bryanchriswhite added a commit that referenced this pull request Jul 11, 2023
@bryanchriswhite
Merge remote-tracking branch 'pokt/main' into fix/zero-value-nonce
7aced4e 
* pokt/main:
  [P2P] Integrate background router (#732)
  Update main README.md
  [Bug] Fix CI linter errors (#885)
  [Tooling] Block `IN_THIS_*` comments from passing CI (#889)
  [Utility] Update E2E feature path template doc (#870)
  [IBC] Add nil check on proof for membership and non-membership proof creation (#877)
  Added git diff state to devlog10
  Devlog 10 (#872)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Olshansk Olshansk Olshansk approved these changes

@h5law h5law Awaiting requested review from h5law

Assignees

@adshmh adshmh

Labels

docs documentation Improvements or additions to documentation small Pull request is small utility Utility specific changes waiting-for-review

Projects

V1 Dashboard
Status: Done

Milestone

M3: Pocket RoS (Relay or Slash)

Development

Successfully merging this pull request may close these issues.

2 participants

@adshmh @Olshansk