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: create and list issues #693

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

kavitha186
Copy link
Collaborator

@kavitha186 kavitha186 commented Feb 4, 2024

This PR is for the feature to post issues on the repositories with shepherd. if you are looking to post issues across multiple repositories, you can use the command "issue" to create, update , list posted issues and close issue

issue with the title, issue message, labels , state and state_reason provided in the migration scripts.

This enhancement feature is for the issue #87 and #590

Screenshots

Create Issue

image image

Update Issue

image

Closing Issue

image image

List Issues

image

Screencasts

Screen-2024-03-17-185925.mp4

@kavitha186 kavitha186 force-pushed the feat-create-issue branch 9 times, most recently from 3b65c3b to f1dcac3 Compare February 8, 2024 02:15
@aorinevo aorinevo changed the base branch from main to feat/create-issues-labels February 11, 2024 18:07
@aorinevo aorinevo changed the base branch from feat/create-issues-labels to main February 11, 2024 18:37
@aorinevo aorinevo changed the base branch from main to feat/create-issues-labels February 11, 2024 18:40
@aorinevo aorinevo self-requested a review February 11, 2024 18:41
@aorinevo aorinevo self-assigned this Feb 11, 2024
@aorinevo aorinevo added the enhancement New feature or request label Feb 11, 2024
@aorinevo
Copy link
Collaborator

aorinevo commented Feb 11, 2024

@kavitha186, we need unit tests to cover issue, list-issues, and list commands. Can you add those?

I've started down this road with checkout.ts. Seems a bit bulky and in need of rework but thought I'd share how I'm thinking about adding coverage for commands.

#697

package.json Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@aorinevo
Copy link
Collaborator

@kavitha186, I've landed test coverage for all the existing commands. You should be good to go to do the same for the new commands introduced in this PR.

@kavitha186 kavitha186 changed the base branch from feat/create-issues-labels to main March 16, 2024 23:17
@kavitha186 kavitha186 force-pushed the feat-create-issue branch 2 times, most recently from 4fa9474 to 614b5e7 Compare March 17, 2024 03:11
@aorinevo
Copy link
Collaborator

@kavitha186, let’s add a screenshot of unit tests coverage. A recording demoing the functionality would be helpful too.

src/services/github.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@TheSavior TheSavior left a comment

Choose a reason for hiding this comment

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

Thanks for tagging me for feedback, I really appreciate the work you all have done here. I really hope this is valuable for you too.

Something I'd really like is to be able to customize the description based on the repo.

For example, say there are 5 issues I'm scanning repos for. If those were autofixable I'd fix them via PR, but they aren't, hence an issue.

So I want to check out the repos, run some scripts, and generate the issue description.

In my example, say I identify that the repo only has 3 of the 5 issues. The issue that I'd file might have some instructions for how to address those 3 issues, and even ideally be able to show the permalink to the lines in question:

adapter:
type: github
search_query: repo:NerdWalletOSS/shepherd-demo path:/ filename:.eslintrc
hooks:
should_migrate:

https://github.com/NerdWalletOSS/shepherd/blob/b1239dd92255d68fd00927fc54d42ea756128d05/examples/eslintrc-yml/shepherd.yml#L3-L7

In order to piece that URL together I'd also need some information. I could probably get the sha by calling git log myself, and I can get the file path / line numbers. Not sure if I'd have the org/repo name though.

@@ -82,6 +82,12 @@ hooks:
post_checkout: npm install
apply: mv .eslintrc .eslintrc.json
pr_message: echo 'Hey! This PR renames `.eslintrc` to `.eslintrc.json`'
issues:
Copy link
Contributor

Choose a reason for hiding this comment

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

What does it mean to have this in the yml as well as the migration for PRs. Would it open a PR and an Issue? Are they mutually exclusive? (I'd kinda expect them to be mutually exclusive). Maybe update the description text above for what this does now

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes they are mutually exclusive

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If you see the list of commands in the ReadMe section, it has description for each command.

  • pr: Creates a PR for each repo with the message generated from the pr_message hook.
  • version: Prints Shepherd version
  • issue: Create, update, or close issues across multiple repository.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@TheSavior, we're exposing issues as a top-level command in Shepherd (i.e., shepherd issues ...). As it is currently implemented, it is mutually exclusive from other commands.

That said, I'm wondering if we should be thinking of incorporating hooks into issues. For example, we can have a should_create_issue and post_create_issue hooks analogous to should_migrate and post_checkout hooks.

@kavitha186 kavitha186 force-pushed the feat-create-issue branch 2 times, most recently from 08bc366 to 5d0fb3a Compare March 24, 2024 04:10
@aorinevo
Copy link
Collaborator

aorinevo commented Mar 30, 2024

@TheSavior and @kavitha186, thank you for your continued feedback and contributions. While I think we've diverged a bit from the requirements Eli is after, I think Kavitha's foundational work is valuable and potentially adjustable to include Eli's use case.

Use Case
To align, let me summarize the use case we're after, leveraging Eli's example. Say there are 5 issues that we are scanning for in repos. If they are auto-fixable, we fix them via a PR. For those issues which are not auto-fixable, we want to create an issue with a description that potentially contains data unique to that repo.

Current State
There are two checkout related hooks that are exposed via Shepherd's migration-spec file: should_migrate and post_checkout. The should_migrate hook executes when a repo is checked out to determine whether to keep the repo. If successful the repo is kept and subsequently the post_checkout hook is executed. If should_migrate is unsuccessful, the repo is removed and post_checkout is not executed.

If post_checkout is successful, the repo is kept. Otherwise, the repo is removed.

Solution 1 (Recommended)
Effort: Low

  1. Leverage should_migrate
    a. Instead of checking strictly whether or not the repo is auto-fixable for all 5 issues, we leverage this hook to generate the data to feed into the post_checkout step. To make the data available between hooks, we can persist state to disk.
  2. In the post_checkout step, we can read in the data and use it to take decisions on what issues to create and incorporate repo specific data into the issue details.

Pros: No code change required to Shepherd
Cons: End user has to persist and manage state on disk

Solutions 2
Effort: Medium

  1. Similar to the recommended solution but we expose a singleton that can be used to pass state between hooks.

Pros: End user does not need to persist and manage state on disk
Cons: This state will only be accessible to hooks attached to the same command.

Solution 3
Effort: High

  1. Similar to Solution 2 but Shepherd persists and manages state on disk.

Pros:

 - End user does not need to persist and manage state on disk.
 - End user can leverage state across hooks and commands.

Cons:

 - We take a performance hit by reading/writing to disk.

@aorinevo
Copy link
Collaborator

@kavitha186, while I did some research on existing tooling to ensure we're not overlapping with feature sets already available elsewhere, I think it would be valuable to double check from your end.

For example, does gh cli tool offer issue creation and management across multiple repos?

Just curious if there is anything we might have missed.

@kavitha186
Copy link
Collaborator Author

kavitha186 commented Mar 30, 2024

@kavitha186, while I did some research on existing tooling to ensure we're not overlapping with feature sets already available elsewhere, I think it would be valuable to double check from your end.

For example, does gh cli tool offer issue creation and management across multiple repos?

Just curious if there is anything we might have missed.

I see there is a gh cli command to post issue or create PRs. Perhaps that is available only to manage a single repo as far as I see.

@TheSavior
Copy link
Contributor

Option 1 seems fine tbh, happy to persist things to dish. It seems like there is some information I couldn't easily get from just a user script running in should_migrate though. For example, in order to form those URLs, I'd need to know the org/repo name. I could probably shell out to git remote and figure it out, would that be your recommendation? I would also probably need that to key the data stored on disk to the right repo so that when the issue hook runs later I know where to look for that persisted repo info.

@TheSavior
Copy link
Contributor

Oh or maybe your point about the gh cli is that I should just use shepherd to search and filter the repos, and once I get to should migrate and run the custom script, that script should just call the gh cli. That would probably work too!

@NerdWalletOSS NerdWalletOSS deleted a comment from kavitha186 Mar 30, 2024
@NerdWalletOSS NerdWalletOSS deleted a comment from kavitha186 Mar 30, 2024
@NerdWalletOSS NerdWalletOSS deleted a comment from kavitha186 Mar 30, 2024
@NerdWalletOSS NerdWalletOSS deleted a comment from kavitha186 Mar 30, 2024
@NerdWalletOSS NerdWalletOSS deleted a comment from kavitha186 Mar 30, 2024
@NerdWalletOSS NerdWalletOSS deleted a comment from kavitha186 Mar 30, 2024
@aorinevo
Copy link
Collaborator

aorinevo commented Mar 30, 2024

Oh or maybe your point about the gh cli is that I should just use shepherd to search and filter the repos, and once I get to should migrate and run the custom script, that script should just call the gh cli. That would probably work too!

My comment is with respect to adding the issues command to Shepherd. That said, I think calling gh within your shepherd scripts would suffice for your use case.

@kavitha186
Copy link
Collaborator Author

image

@kavitha186 kavitha186 force-pushed the feat-create-issue branch 2 times, most recently from 859aa60 to b104d76 Compare April 6, 2024 23:13
@aorinevo
Copy link
Collaborator

@kavitha186, can we address the CI failures?

@aorinevo
Copy link
Collaborator

@nwalters512, would love your input and feedback on this PR when you get a chance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants