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

dev-ux: create an issue for each TODO introduced in a merged PR #6636

Closed
wants to merge 3 commits into from

Conversation

pysel
Copy link
Member

@pysel pysel commented Oct 5, 2023

Closes: #6399

What is the purpose of the change

Add a workflow that runs when a PR is merged to main, checking if the PR has some TODOs left by searching for TODO: <something> pattern. If found, creates an issue assigned to the author of an initial PR with a title <something>.

Also, for convenience, attaches a T:auto label to this issue

Note: if this PR gets merged, I suggest people start avoiding leaving TODOs like "fix this", "refactor this", etc, because it would make little sense in the issue title

Testing and Verifying

Fork

Documentation and Release Note

NA

@pysel pysel added T:dev-UX V:state/compatible/no_backport State machine compatible PR, depends on prior breaks github_actions Pull requests that update GitHub Actions code A:no-changelog labels Oct 5, 2023
@github-actions github-actions bot added the T:CI label Oct 5, 2023
id: todos
run: |
git fetch origin main
todos=$(git diff ${{ github.event.pull_request.base.sha }}..${{ github.event.pull_request.merge_commit_sha }} --unified=0 | awk '/^\+.*TODO:/ { sub(/^.*TODO:\s*/, ""); sub(/^ */, ""); $0=toupper(substr($0,1,1)) substr($0,2); print $0 }')
Copy link
Member Author

Choose a reason for hiding this comment

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

note: this line greps "TODO:"-patterned lines from a diff introduced by a merged pull request. It does that by only checking lines starting with + sign, since git diff marks added files with a plus in the output. Also, it removes all whitespaces after : and capitalizes a first non-whitespace character

Comment on lines +34 to +37
SAVEIFS=$IFS # Save current IFS (Internal Field Separator)
IFS=$'\n' # Change IFS to newline char
todos=("${{ steps.todos.outputs.todo_list }}") # split the `todo_list` string into an array by the same name
IFS=$SAVEIFS # Restore original IFS
Copy link
Member Author

Choose a reason for hiding this comment

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

note: this is just a way of converting todo_list from string with newlines to an array so we could iterate over todos

Comment on lines +42 to +47
curl -L \
-X POST \
-H "Accept: application/vnd.github+json" \
-H "Authorization: token ${{ secrets.GITHUB_TOKEN }}" \
https://api.github.com/repos/${{ github.repository }}/issues \
-d '{"title":"'"$todo"'","body":"Auto-generated issue triggered by the merge of ${{ github.event.pull_request.html_url }} with an introduced TODO","assignees":["${{ github.event.pull_request.user.login }}"],"labels":["T:auto"]}'
Copy link
Member Author

Choose a reason for hiding this comment

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

note: unfortunately it is not possible to use an existing github action for issue creation, since a PR can potentially introduce multiple TODOs. That is because we need to iterate over all of them, but as of my knowledge, if we used an existing action for opening an issue, we would have to iterate over a workflow's step, which is not currently possible

@pysel pysel marked this pull request as ready for review October 5, 2023 01:08
@pysel pysel requested a review from a team as a code owner October 5, 2023 01:08
@pysel pysel requested review from p0mvn and mattverse October 5, 2023 01:08
@ValarDragon
Copy link
Member

I don't really want this, but if other people want it happy to try

@mattverse
Copy link
Member

Is there a sample issue that was created by this workflow?

@p0mvn
Copy link
Member

p0mvn commented Oct 5, 2023

I don't really want this, but if other people want it happy to try

I would find it useful. If folks disagree, can we make this actionable on a label - "A:TODO"?

CC: @pysel

@pysel
Copy link
Member Author

pysel commented Oct 5, 2023

Is there a sample issue that was created by this workflow?

sure, you can check Testing and Veryfing section of this PR

@pysel
Copy link
Member Author

pysel commented Oct 5, 2023

I don't really want this, but if other people want it happy to try

I would find it useful. If folks disagree, can we make this actionable on a label - "A:TODO"?

CC: @pysel

yes, I was also thinking about that

@czarcas7ic
Copy link
Member

I think a variation of this would actually be nice:

Require that any TODO that is merged has an in line comment below it with the github issue linked. I think its better than just making a cookie cutter issue for it

@mattverse
Copy link
Member

I am with @czarcas7ic 's idea on this: although it would add one more step for us that would be more uncomfortable, I do think its needed + good practice

@pysel pysel marked this pull request as draft October 10, 2023 02:21
@pysel
Copy link
Member Author

pysel commented Oct 10, 2023

I think a variation of this would actually be nice:

Require that any TODO that is merged has an in line comment below it with the github issue linked. I think its better than just making a cookie cutter issue for it

@czarcas7ic should we then close this PR and go with a different workflow, or should we merge them together and automatically append a link of a newly created issue after TODO line?

@czarcas7ic
Copy link
Member

czarcas7ic commented Oct 10, 2023

@pysel I think the latter might be too much automation if I understand you correctly. I think the best flow would be as follows:

  1. CI checks that there is a // TODO that was added
  2. CI checks that below that line there is some text that includes "github.com/osmosis-labs/osmosis/issues"
    2a. If the text is there, succeed
    2b. If not, error with the line and explain that an issue must be added if a TODO is added

Apologize for making this work go to waste, I just think the above flow will work a lot better for everyone

@pysel pysel closed this Oct 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:no-changelog github_actions Pull requests that update GitHub Actions code T:CI T:dev-UX V:state/compatible/no_backport State machine compatible PR, depends on prior breaks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ci: create issue for each TODO in the PR diff on merge
5 participants