Add github package #32

damccorm · 2019-07-23T19:44:06Z

Resolves #23

stephenmichaelf · 2019-07-23T21:20:53Z

Could you provide some details on this? What is the background/why? Thanks!

damccorm · 2019-07-23T21:30:17Z

Sure. A version of this existed in the old toolkit. Basically, the goal here is to make it easy for users to know what the context of their action is (e.g. on issue being opened, or PR, or commit etc...) and to interact with Git from actions. I know of at least one user who specifically asked about this as well, I think people were using it in V1

damccorm · 2019-07-23T21:30:38Z

We have all this info on the runner already, this is mostly just about exposing it

bryanmacfarlane · 2019-07-23T21:39:49Z

The background is SDLC type actions (on issue create etc.) that get an octakit client hydrated with the context. Think probot for actions: https://github.com/probot/probot

JasonEtco · 2019-07-23T21:43:59Z

I wrote a blog post on building Actions in Node.js in which I elaborate on what it looks like to build them with and without actions-toolkit - the goal was generally to expose information that almost every action needs to better understand the "context" that its acting under, and to provide an API that is more approachable/defined than accessing environment variables.

bryanmacfarlane · 2019-07-24T03:40:18Z

We need to validate for container and VM/script actions. both should "just work"

bryanmacfarlane · 2019-07-24T03:43:00Z

packages/github/README.md

+> Classes for accessing the GitHub context and a hydrated GitHub client.
+
+## Usage
+


This should include some inspirational getting started code on how easy it is to use. See probot readme for a good example.

We should probably create a hello world sample on the github graph and reference it here.

Added some guidance here, can add a hello world sample as well, maybe makes sense to wait until this is merged though (in case anything changes here)? I think its a good idea, guessing we'd want that repo to live in the actions org as well

packages/github/package.json

bryanmacfarlane · 2019-07-24T03:44:13Z

packages/github/package.json

+  },
+  "scripts": {
+    "test": "echo \"Error: run tests from root\" && exit 1",
+    "tsc": "tsc"


Lerna needs the tsc alias, added a build alias to the same thing though.

bryanmacfarlane · 2019-07-24T03:58:31Z

I wrote up a bunch of questions / thoughts and then I read the blog post ;) . I think we should follow that pattern where it's actions authors job to do if (context.payload.{eventname}) { code ...}

damccorm · 2019-07-24T18:37:00Z

We need to validate for container and VM/script actions. both should "just work"

Yeah, this should work fine for both. All the environment variables we need are persisted, just made sure of that.

packages/github/__tests__/lib.test.ts

JasonEtco · 2019-07-25T14:43:50Z

packages/github/README.md

+// This should be a token with access to your repository scoped in as a secret.
+const myToken = process.env.GITHUB_TOKEN
+
+const octokit = new github.GitHub(myToken)


Part of the charm of actions-toolkit (and Probot) is that requests made through Octokit are authenticated by default; perhaps the first argument of GitHub could be set to process.env.GITHUB_TOKEN by default?

const octokit = new github.GitHub() class GitHub { constructor (token = process.env.GITHUB_TOKEN) {} }

Or even better, github is just an instance of GitHub - something like this (but in TS obviously):

module.exports = new GitHub(process.env.GITHUB_TOKEN module.exports.GitHub = GitHub

We don't pass that in from the runner by default (for security reasons, I believe, so that we don't implicitly trust all actions with this info) so it feels to me like this is just placing a burden back on the actions author.

From an end user perspective, its probably not going to look any different, they're probably going to have to scope their token in to the action through an input regardless, its just the difference between the action author setting process.env.GITHUB_TOKEN or passing it in as an input here.

Unless I'm missing something, I'd lean towards keeping it as is.

In Actions v1, users had to explicitly include the token via the secrets property:

action "Some action" { secrets = ["GITHUB_TOKEN"] }

So I don't feel like its too different here. I think that in practice, users will be including this package so that they can make API requests, most of the time using the provided token. The benefit to having it set by default in the lib is that they don't have to include it twice (the workflow config and the code). I think that having it "just work" is what makes it feel easy, and even this small step takes that magic away

I don't really love this pattern. This requires the user of the action to know that the action requires that secret being passed in as an env variable, and as far as I know there's not a way for the action author to present that dependency (other than throwing if they don't supply the token).

The pattern I'd prefer to encourage is the author taking the token as an input. That way the end user can simply look at the inputs for the task and react accordingly. The toolkit also does all the work of erroring if its not supplied, which I think offsets the (IMO very small) burden we're putting on the author.

Thoughts?

I think that's fair. You hit something really related there, throwing an error if the expected arguments/secrets/whatever aren't provided - it'd be really nice to have something in the platform itself that actions can use, but that's a different conversation.

I feel like this one choice separates Actions from being a "scriptable GitHub" and being a general purpose runtime; by providing a pre-authenticated SDK, the action's author doesn't have to think about authentication at all and can just start integrating with GitHub. Without it, there is definitely more explicit setup (which is good) - but then that'd be true for any SDK that can be used within Actions.

So - I guess it boils down to whether or not we'd want the GitHub SDK to have that sort of special, already-setup behavior, or if we favor being more explicit about the setup. I see tradeoffs for both (and appreciate hearing your perspective! ❤️)

@jclem, @bryanmacfarlane, @stephenmichaelf any thoughts? A 3rd perspective might be helpful here.

Given our model is explicit inputs I would rather see the developer of the action explicitly take the input of the token rather than requiring the user to pas the token as part of the environment. Because of this, we can't just default to process.env.GITHUB_TOKEN.

I think the model for Actions to be explicit about passing secrets into the actions is, in general, a good thing, however, it does place some significant burden on the consumer of the action to constantly add the ${{ secrets.GITHUB_TOKEN }} to pass it in makes for some friction.

If we had a better way for customers to know that an action wanted permissions for the GITHUB_TOKEN and the customer could approve that in some way we may have better options. However, I don't think that we should require the consumer of the action to dig into the actions code to see if it wants the token as that token is quite powerful.

If we had a better way for customers to know that an action wanted permissions for the GITHUB_TOKEN and the customer could approve that in some way we may have better options.

I agree! https://github.com/github/actions-project/issues/783

I don't have access to actions-project 😢

Regardless, I think until we do have a way to do that we should proceed with this as coded now - we can add that change in the future without breaking back-compat if needed. Does that seem right?

packages/github/README.md

packages/github/__tests__/lib.test.ts

packages/github/src/context.ts

stephenmichaelf · 2019-07-26T13:17:51Z

packages/github/src/context.ts

+    this.actor = process.env.GITHUB_ACTOR as string
+  }
+
+  get issue(): {owner: string; repo: string; number: number} {


Is it common to have the return types structured like this? Personal preference is to define types.

So as I think about this, I think I like it as is. The reasoning is that if we were going to create an interface for this, the issue interface that would apply here doesn't exactly match the interface used in the payload (which can take extra key/value pairs).

Now that initially doesn't seem right, but it actually makes sense - what we're getting here is what the rest client expects, what we're getting in the payload is everything GitHub is passing - they don't match exactly.

So I'd be in favor of leaving this as is, rather than creating an issue interface that only applies in one place. Make sense? Thoughts?

It doesn't necessarily have to be the same interface everywhere for it to be valuable. From the context of the user's perspective, it's going to look pretty rough if they have strict compilation on (which I think is recommended now). I don't think it's block worthy. Maybe good to get some feedback from others so that we have a shared idea of which way we want to go and can use pattern in other places.

@JasonEtco

stephenmichaelf · 2019-07-26T13:19:50Z

packages/github/src/interfaces.ts

+export interface WebhookPayloadWithRepository {
+  [key: string]: any
+  repository?: PayloadRepository
+  issue?: {


Same question here about types.

See previous response.

damccorm · 2019-07-29T13:04:35Z

Any objections to merging this? Would love to get it in today since we have at least one guy on our side getting ready to write some SDLC actions that would presumably use this.

stephenmichaelf · 2019-07-29T14:23:07Z

I am OK merging, should get approval from others too.

JasonEtco

I'm good with this being merged! On this comment about the GITHUB_TOKEN:

we can add that change in the future without breaking back-compat if needed.

This is a really great point, so I'm 👍 to 🚢!

Danny McCormick added 2 commits July 23, 2019 15:40

Add github package

7be75e7

Docs

6d83d2f

damccorm requested review from jclem, stephenmichaelf, bryanmacfarlane and JasonEtco July 23, 2019 19:44

bryanmacfarlane reviewed Jul 24, 2019

View reviewed changes

packages/github/package.json Outdated Show resolved Hide resolved

bryanmacfarlane reviewed Jul 24, 2019

View reviewed changes

Danny McCormick added 3 commits July 24, 2019 16:59

Feedback

f2f748f

Accidentally added extra

f48c889

Allow octokit to be extended

1786ff4

JasonEtco reviewed Jul 25, 2019

View reviewed changes

Respond to feedback

0583f7a

stephenmichaelf reviewed Jul 26, 2019

View reviewed changes

packages/github/src/context.ts Outdated Show resolved Hide resolved

stephenmichaelf reviewed Jul 26, 2019

View reviewed changes

packages/github/src/context.ts Outdated Show resolved Hide resolved

stephenmichaelf reviewed Jul 26, 2019

View reviewed changes

Feedback

954f621

JasonEtco approved these changes Jul 29, 2019

View reviewed changes

damccorm merged commit 2a2b51f into master Jul 29, 2019

damccorm deleted the features/github branch July 29, 2019 17:09

falkenhawk mentioned this pull request Dec 29, 2022

Add php 8.0 to GitHub Actions ignoring its errors zf1s/zf1#59

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add github package #32

Add github package #32

damccorm commented Jul 23, 2019 •

edited

Loading

stephenmichaelf commented Jul 23, 2019

damccorm commented Jul 23, 2019

damccorm commented Jul 23, 2019

bryanmacfarlane commented Jul 23, 2019

JasonEtco commented Jul 23, 2019

bryanmacfarlane commented Jul 24, 2019 •

edited

Loading

bryanmacfarlane Jul 24, 2019

damccorm Jul 25, 2019

bryanmacfarlane Jul 24, 2019

damccorm Jul 25, 2019

bryanmacfarlane commented Jul 24, 2019

damccorm commented Jul 24, 2019

JasonEtco Jul 25, 2019

damccorm Jul 25, 2019 •

edited

Loading

JasonEtco Jul 25, 2019

damccorm Jul 25, 2019

JasonEtco Jul 25, 2019

damccorm Jul 25, 2019

chrispat Jul 26, 2019

JasonEtco Jul 26, 2019

damccorm Jul 26, 2019

damccorm Jul 26, 2019 •

edited

Loading

stephenmichaelf Jul 26, 2019

damccorm Jul 26, 2019

stephenmichaelf Jul 29, 2019

stephenmichaelf Jul 26, 2019

damccorm Jul 26, 2019

damccorm commented Jul 29, 2019

stephenmichaelf commented Jul 29, 2019

JasonEtco left a comment

		> Classes for accessing the GitHub context and a hydrated GitHub client.

		## Usage

Add github package #32

Add github package #32

Conversation

damccorm commented Jul 23, 2019 • edited Loading

stephenmichaelf commented Jul 23, 2019

damccorm commented Jul 23, 2019

damccorm commented Jul 23, 2019

bryanmacfarlane commented Jul 23, 2019

JasonEtco commented Jul 23, 2019

bryanmacfarlane commented Jul 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryanmacfarlane commented Jul 24, 2019

damccorm commented Jul 24, 2019

Choose a reason for hiding this comment

damccorm Jul 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damccorm Jul 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damccorm commented Jul 29, 2019

stephenmichaelf commented Jul 29, 2019

JasonEtco left a comment

Choose a reason for hiding this comment

damccorm commented Jul 23, 2019 •

edited

Loading

bryanmacfarlane commented Jul 24, 2019 •

edited

Loading

damccorm Jul 25, 2019 •

edited

Loading

damccorm Jul 26, 2019 •

edited

Loading