Skip to content

[Proposal] Refactor ng-dev's pr directory to disentangle the utilities and common pieces #203

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

Closed
2 tasks done
josephperrott opened this issue Sep 2, 2021 · 1 comment
Closed
2 tasks done

Comments

@josephperrott
Copy link
Member

josephperrott commented Sep 2, 2021

Currently we have a structure like this.

ng-dev/pr                   
├── checkout                │ Checks out a PR locally to modify
├── check-target-branches   │ Prints the branches a pr would merge into
├── common                  │ 
│   └── checkout-pr.ts      │ Actual logic for checking out a PR used by checkout and rebase
├── discover-new-conflicts  │ Determines what other pending PRs can merge cleanly after a PR merges
├── merge                   │ Merges PRs (also houses the config.ts)
└── rebase                  │ Attempts to rebase a PR and push the rebased commits back to upstream

We have had a few people ask for something akin to ng-dev pr check <pr-number>, which would return the pr title, number, if it needs approvals, what branches it would land in etc. I think we should transform the check-target-branches into this command.

I would propose that we then separate out a few pieces that can be reused both literally in other files/commands and conceptually.

ng-dev/pr                   
├── checkout                │ Stays the same
├── info                    │ Leverage the validation logic and PullRequest object to get all PR info
├── common                  │ 
│   ├── checkout-pr         │ Stays the same
│   ├── get-pr              │ Common util to get PR from github as a consistent PullRequest object
│   ├── targeting           │ Target labels and utils to get a target branch for a PR, etc
│   └── validation          │ Validate that a PR meets all of the expected criteria for the repo
├── discover-new-conflicts  │ Stays the same
├── merge                   │ Merges PR, using the new common utils
└── rebase                  │ Stays the same
  • config.ts should move up one level into ng-dev/pr
  • MergeConfig should be renamed to something like PullRequestConfig
josephperrott added a commit to josephperrott/dev-infra that referenced this issue Sep 16, 2021
As part of angular#203, refactoring the pr directory into more reusable pieces.
josephperrott added a commit to josephperrott/dev-infra that referenced this issue Sep 16, 2021
As part of angular#203, refactoring the pr directory into more reusable pieces.
josephperrott added a commit that referenced this issue Sep 16, 2021
)

As part of #203, refactoring the pr directory into more reusable pieces.

PR Close #225
josephperrott added a commit to josephperrott/dev-infra that referenced this issue Sep 16, 2021
As part of angular#203, refactoring the pr directory into more reusable pieces.
josephperrott added a commit to josephperrott/dev-infra that referenced this issue Sep 16, 2021
As part of angular#203, refactoring the pr directory into more reusable pieces.
josephperrott added a commit that referenced this issue Sep 16, 2021
…#227)

As part of #203, refactoring the pr directory into more reusable pieces.

PR Close #227
josephperrott added a commit to josephperrott/dev-infra that referenced this issue Sep 17, 2021
…common folder

As part of angular#203, refactoring the pr directory into more reusable pieces
@josephperrott josephperrott self-assigned this Sep 17, 2021
josephperrott added a commit to josephperrott/dev-infra that referenced this issue Sep 17, 2021
…common folder

As part of angular#203, refactoring the pr directory into more reusable pieces
josephperrott added a commit that referenced this issue Sep 17, 2021
…common folder (#233)

As part of #203, refactoring the pr directory into more reusable pieces

PR Close #233
josephperrott added a commit to josephperrott/dev-infra that referenced this issue Sep 20, 2021
… attribute at pullRequest instead of merge

Rather than using MergeConfig from merge in the ng-dev config object, we use PullRequestConfig from pullRequest
as this config applies to all `pr` commands.

Fixes angular#203

BREAKING CHANGE:
`MergeConfig` has been renamed to `PullRequestConfig` and is now accessed via `pullRequest` on the provided
ng-dev config.
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Oct 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

1 participant