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

Improve compatabilty with classic probot patterns #24

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jbjonesjr
Copy link

Recently, i've been migrating some classic probot apps to run on top of the GitHub Actions platform, and found this adapter to be a huge help in simplifying that process.

However, I've found that there are some core assumptions that are built into probot usage patterns and functions that break with:

  • the GitHub Actions event model and payloads
  • Running Probot apps outside of a GitHub App contianer.

This pull request attempts to make some additional changes in adapter-github-actions or discusses potential changes in other packages to simplify this migration process.

Copy link
Author

Item 1:

The schedule event:

The ability to run an action on a timer/cron is a great capability that simplifies running an app. This allows us to deprecate the need of probot scheduler in place of this native functionality.

A few options present themselves with this opportunity:

  • Align the events thrown by probot-scheduler to those thrown by GitHub Actions to simplify migration paths. The reality is that not all apps are going to be able to be run in an actions environment (especially for some large Enterprises), so continuing to support scheduler in it's bare minimum environment (as is today in the archived form ) is reasonable.
  • The payload thrown by a GitHub Action schedule event is very sparse compared to what is thrown by the probot-scheduler action. There is not event.payload.repository information for example, despite that being available in most (all?) cases via the GITHUB_REPOSITORY environment variable. Should we add a pre-processor or helper method to backfill this information?

When the schedule event is fired by GitHub Actions

Copy link
Author

jbjonesjr commented Feb 28, 2021

Item 2

GitHub App vs GitHub Action

GitHub Apps provided an installation ID to uniquely identify installations. This was used by plugins and apps (such as probot/metadata for example) to uniquely identify the instance. Should this be moved to a more generic model that can return either a unique APP ID or something like GITHUB_ACTION environment variable that is listed as a unique ACTION in?

Copy link
Author

jbjonesjr commented Feb 28, 2021

Item 3:

Octokit access

I admittedly haven't done as much research here, but know it posed a problem on the schedule event, so am extrapolating it's a wider issue.
@octokit/action is a great way to authenticate and configure Octokit for GitHub Actions environments. Should the adapter provides a functioning octokit instance available at context.(github|octokit)?

Copy link
Author

jbjonesjr commented Feb 28, 2021

Item 4

What does a hybrid app look like?

This is what i'm looking at today. Does this make sense, can we optimize elsewhere?

const { run } = require('@probot/adapter-github-actions')
const { probotRun } = require('probot')
const actionApp = require('./app-action')
const probotApp = require('./app')

if (provess.env.GITHUB_ACTIONS) {
    run(actionApp).catch((error) => {
        console.error(error);
        process.exit(1);
    });
} else {
    probotRun(probotApp).catch((error) => {
        console.error(error);
        process.exit(1);
    });
}

@gr2m
Copy link
Contributor

gr2m commented Feb 28, 2021

Recently, I've been migrating some classic probot apps to run on top of the GitHub Actions platform, and found this adapter to be a huge help in simplifying that process

yay, glad to hear that it works for you!

The schedule event:

The ability to run an action on a timer/cron is a great capability that simplifies running an app. This allows us to deprecate the need of probot scheduler in place of this native functionality.

A few options present themselves with this opportunity:

  • Align the events thrown by probot-scheduler to those thrown by GitHub Actions to simplify migration paths. The reality is that not all apps are going to be able to be run in an actions environment (especially for some large Enterprises), so continuing to support scheduler in it's bare minimum environment (as is today in the archived form ) is reasonable.

We archived probot-scheduler quite a while ago, I'm would rather not add changes to it. But if there is demand by GitHub Enterprise Server users and you'd like to do the work yourself with the goal to help migrate to @probot/adapter-github-actions, I'd be happy to support you.

  • The payload thrown by a GitHub Action schedule event is very sparse compared to what is thrown by the probot-scheduler action. There is not event.payload.repository information for example, despite that being available in most (all?) cases via the GITHUB_REPOSITORY environment variable. Should we add a pre-processor or helper method to backfill this information?

I'm reluctant to transparently amend the event payload, it's not something we do on in other circumstances.

An alternative we might consider

The probot application function accepts a 2nd argument which is an object that can accept context-specific options. That's how the Probot Server passes the getRouter() method: https://probot.github.io/docs/http/. When using the Actions adapter we could use it to pass repository:

async function app(app, { repository }) => {})

I'm not saying it's better overall, just an alternative. I like that it's more explicit and not doing magic things in the background that would be hard to debug if something didn't work as expected, e.g. when creating tests.

Item 2

GitHub App vs GitHub Action

GitHub Apps provided an installation ID to uniquely identify installations. This was used by plugins and apps (such as probot/metadata for example) to uniquely identify the instance. Should this be moved to a more generic model that can return either a unique APP ID or something like GITHUB_ACTION environment variable that is listed as a unique ACTION in?

I'm not sure about this one. We cannot just make up an installation ID, it is used for authentication. If anything we should set it to something like [NOT AVAILABLE IN GITHUB ACTIONS: git.io/123] with git.io/123 linking to an in-depth explanation somewhere.

Item 3:

Octokit access

I admittedly haven't done as much research here, but know it posed a problem on the schedule event, so am extrapolating it's a wider issue.
@octokit/action is a great way to authenticate and configure Octokit for GitHub Actions environments. Should the adapter provides a functioning octokit instance available at context.(github|octokit)?

context.octokit should be set? Isn't it working right now?

In future we should also get app.octokit which will work like app.octokit in @octokit/app. When run in the context of a GitHub Action, the instance will be authenticated using the token, so e.g. app.octokit.request("GET /app") will fail with an explanation.

In Probot 11, you should be able to achive the same with

const octokit = app.auth()

Item 4

What does a hybrid app look like?

This is what i'm looking at today. Does this make sense, can we optimize elsewhere?

const { run } = require('@probot/adapter-github-actions')
const { probotRun } = require('probot')
const actionApp = require('./app-action')
const probotApp = require('./app')

if (provess.env.GITHUB_ACTIONS) {
    run(actionApp).catch((error) => {
        console.error(error);
        process.exit(1);
    });
} else {
    probotRun(probotApp).catch((error) => {
        console.error(error);
        process.exit(1);
    });
}

Why do you have the same entrypoint for GitHub Action and other environments?

I'd probably use node action.js when running the app in a GitHub Action, and probot run ./app.js when running the app as a long living node process.

Probot
GitHub Apps to automate and improve your workflow

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

Successfully merging this pull request may close these issues.

2 participants