Skip to content
This repository was archived by the owner on Sep 1, 2022. It is now read-only.

Replace all mentions of the octokit action #24

Merged
merged 13 commits into from
Jul 31, 2019
Merged

Conversation

hectorsector
Copy link
Member

This PR replaces all uses of the octokit action for suggested changes with createPullRequestComment.

This PR will be ready to merge when all the octokit references in config.yml have been replaced.

Closes #21.

@ghost ghost temporarily deployed to production July 30, 2019 19:53 Inactive
@github-learning-lab github-learning-lab bot temporarily deployed to production July 30, 2019 19:53 Inactive
@ghost ghost temporarily deployed to production July 30, 2019 20:02 Inactive
@github-learning-lab github-learning-lab bot temporarily deployed to production July 30, 2019 20:02 Inactive
@ghost ghost temporarily deployed to production July 30, 2019 20:09 Inactive
@github-learning-lab github-learning-lab bot temporarily deployed to production July 30, 2019 20:09 Inactive
@ghost ghost temporarily deployed to production July 30, 2019 20:19 Inactive
@github-learning-lab github-learning-lab bot temporarily deployed to production July 30, 2019 20:19 Inactive
@ghost ghost temporarily deployed to production July 30, 2019 20:24 Inactive
@github-learning-lab github-learning-lab bot temporarily deployed to production July 30, 2019 20:24 Inactive
@hectorsector hectorsector requested a review from partyshah July 30, 2019 20:24
@hectorsector
Copy link
Member Author

@partyshah this is ready for 👀. Some things I'd love for you to review:

  • test that the course works and suggested changes are dropped as expected
  • file names are what you want

position: 20
pullRequest: '%actions.metaPR2.data.number%'

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

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.

Copy link
Member Author

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

@ghost ghost temporarily deployed to production July 30, 2019 21:28 Inactive
@github-learning-lab github-learning-lab bot temporarily deployed to production July 30, 2019 21:28 Inactive
@partyshah
Copy link

@hectorsector

When I enroll in this course and open the first PR, a debug error is thrown:

https://github.com/partyshah/intro-react/issues/3

Scroll down for the next step.
- type: createPullRequestComment
body: import-child-component-activity.md
file: src/App.jsx
Copy link
Member Author

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

Copy link
Member Author

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?

@ghost ghost temporarily deployed to production July 31, 2019 20:23 Inactive
@github-learning-lab github-learning-lab bot temporarily deployed to production July 31, 2019 20:23 Inactive
@partyshah
Copy link

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%'
Copy link
Member Author

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:

  1. There's no metaPR2 action_id in this step, and
  2. Because the event for this step is pull_request.opened, we actually don't need to get the pull request at all.

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.

@github-learning-lab github-learning-lab bot temporarily deployed to production July 31, 2019 21:20 Inactive
@ghost ghost temporarily deployed to production July 31, 2019 21:20 Inactive
@ghost ghost temporarily deployed to production July 31, 2019 22:05 Inactive
@github-learning-lab github-learning-lab bot temporarily deployed to production July 31, 2019 22:05 Inactive
Copy link
Member Author

@hectorsector hectorsector left a comment

Choose a reason for hiding this comment

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

:shipit: looks ✨ @partyshah. Want to do the honors?

@partyshah partyshah merged commit 3f6db69 into master Jul 31, 2019
@partyshah partyshah deleted the replace-octokit branch July 31, 2019 22:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change from octokit to normal action
2 participants