-
Notifications
You must be signed in to change notification settings - Fork 10
Replace all mentions of the octokit action #24
Conversation
@partyshah this is ready for 👀. Some things I'd love for you to review:
|
position: 20 | ||
pullRequest: '%actions.metaPR2.data.number%' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for clarification here, why do we have this? I thought we wouldn't need this since we are not using octokit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this a couple of times later in the file as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great question! We need to explicitly state the issue or PR when the event
for the step doesn't provide it.
For this step, it's event: push
. A push
's payload doesn't contain any information about associated pull requests. Therefore, we need to explicitly state which pull request we want to leave the comment in.
Here's an example where event: pull_request.closed
. In this example, the pull_request
's payload does indeed contain information about where to respond, so the action doesn't need to explicitly state the pullRequest.
There are a lot of ways we can reference the PR. You've done it by grabbing the pull request from the API, and using that pull request's number. You can also do it by title, since you know it is supposed to be called "Changes".
When I enroll in this course and open the first PR, a debug error is thrown: |
Scroll down for the next step. | ||
- type: createPullRequestComment | ||
body: import-child-component-activity.md | ||
file: src/App.jsx |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@partyshah I think this action needs a pullRequest
field for the same reason we discussed above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@partyshah we didn't fix this ☝️ did we?
Fixed conflicts. @hectorsector |
config.yml
Outdated
@@ -34,6 +34,7 @@ steps: | |||
body: header-component-activity.md | |||
file: src/App.jsx | |||
position: 85 | |||
pullRequest: '%actions.metaPR2.data.number%' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strike my last comment, @partyshah. This field is problematic for a couple of reasons:
- There's no
metaPR2
action_id in this step, and - Because the event for this step is
pull_request.opened
, we actually don't need to get the pull request at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. I'll fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks ✨ @partyshah. Want to do the honors?
This PR replaces all uses of the
octokit
action for suggested changes withcreatePullRequestComment
.This PR will be ready to merge when all the
octokit
references inconfig.yml
have been replaced.Closes #21.