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

feat(package): publish the app as an npm package #847

Merged
merged 6 commits into from Jan 16, 2024
Merged

feat(package): publish the app as an npm package #847

merged 6 commits into from Jan 16, 2024

Conversation

travi
Copy link
Member

@travi travi commented Jan 12, 2024

No description provided.

travi and others added 4 commits January 4, 2024 10:54
to improve the ability to include into an existing Probot app as a
dependency

Co-authored-by: Julie Van Kirk <VanKirkJulieA@JohnDeere.com>
Co-authored-by: Douglas Johnson <JohnsonDouglas@JohnDeere.com>
Co-authored-by: Julie Van Kirk <VanKirkJulieA@JohnDeere.com>
Co-authored-by: Douglas Johnson <JohnsonDouglas@JohnDeere.com>
Co-authored-by: Julie Van Kirk <VanKirkJulieA@JohnDeere.com>
Co-authored-by: Douglas Johnson <JohnsonDouglas@JohnDeere.com>
Co-authored-by: Julie Van Kirk <VanKirkJulieA@JohnDeere.com>
Co-authored-by: Douglas Johnson <JohnsonDouglas@JohnDeere.com>
@travi travi requested a review from gr2m January 12, 2024 20:58
Copy link

vercel bot commented Jan 12, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
app ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 13, 2024 3:37am

Copy link
Member Author

Choose a reason for hiding this comment

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

i plan to give more attention to improving the details in this doc, but wanted to get it moved to its own file so that the readme was less noisy and folks can find their way to getting started more easily

Comment on lines +25 to +60
```js
import {Server, Probot, ProbotOctokit} from 'probot';
import app from '@repository-settings/app';

async function start() {
const log = getLog();
const server = new Server({
Probot: Probot.defaults({
appId: process.env.APP_ID,
privateKey: process.env.PRIVATE_KEY,
secret: process.env.WEBHOOK_SECRET,
Octokit: ProbotOctokit.defaults({
baseUrl: process.env.GH_API
}),
log: log.child({
name: 'repository-settings'
})
}),
log: log.child({
name: 'server'
})
});

process.on('SIGTERM', async () => {
console.log('Received SIGTERM, stopping server!');

await server.stop();
});

await server.load(app);

await server.start();
};

start();
```
Copy link
Member Author

Choose a reason for hiding this comment

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

interested in whether you think there is a simpler example that would help folks understand the goal, or even a more generic probot doc to link to, but wanted to provide some level of guidance here

Copy link
Member

Choose a reason for hiding this comment

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

I think if you don't need any custom functionality and you want a long-running process, I think there is no code necessary at all, you should be able to do "start": "probot run node_modules/@repository-settings/app/index.js", or maybe @repository-settings/app should expos its own binary like "start": "repository-settings"?

If users need more customization, for example for a serverless environment, then we can point them to https://probot.github.io/docs/development/#run-probot-programmatically and maybe provide an example for a serverless app?

Copy link
Member Author

Choose a reason for hiding this comment

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

i like the idea of including a few different variations from simplest to most customizability.

maybe @repository-settings/app should expos its own binary like "start": "repository-settings"?

i like this idea

npm install @repository-settings/app
```

#### Example node.js app
Copy link
Member Author

Choose a reason for hiding this comment

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

in addition to this sort of example, i could see value in providing a minimal example for deploying to a serverless environment. i dont have as much experience with that approach lately, so open to suggestions. i dont think adding that needs to block getting this progress merged, but could be a valuable follow up

Copy link
Member

Choose a reason for hiding this comment

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

I think it's a good idea, but we can do a follow up for that.

Comment on lines +66 to +86
## Permissions & events

This plugin requires these **Permissions & events** for the GitHub Integration:

### Permissions

- Administration: **Read & Write**
- Contents: **Read only**
- Issues: **Read & Write**
- Single file: **Read & Write**
- Path: `.github/settings.yml`
- Actions: **Read only**

### Organization Permissions

- Members: **Read & Write**

### Events

- Push
- Repository
Copy link
Member Author

Choose a reason for hiding this comment

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

i don't know how accurate these even still are, but wanted to get them moved into this doc as-is for now. can adjust for correctness as a follow up. i also realized that we also havent added the file to enable the manifest flow, which is probably the better source of truth to maintain, so maybe that is the better follow up here

@@ -1,25 +0,0 @@
# Deploying

If you would like to run your own instance of this plugin, see the [docs for deploying plugins](https://github.com/probot/probot/blob/master/docs/deployment.md).
Copy link
Member Author

Choose a reason for hiding this comment

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

i dont think the plugins approach is even valid anymore, so i captured the idea of this more as just forking the repo.

Copy link
Member

@gr2m gr2m left a comment

Choose a reason for hiding this comment

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

I couldn't try out the code but from looking at it this is great, I'd move forward and then iterate on it as needed

* [Hosted GitHub.com App](#hosted-githubcom-app)
* [Self-Hosted App](#self-hosted-app)
* [Configuration](#configuration)
* [Security Implications](#security-implications)
Copy link
Member

Choose a reason for hiding this comment

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

I usually leave out a TOC these days, because GitHub has its own build into the UI now, and unless updating a TOC in a markdown file is automated it tends to get out of sync with the actually sections.

Copy link
Member Author

Choose a reason for hiding this comment

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

thats fair. my reason for including is usually more for drawing attention to sections below the fold. i've known about the built-toc, but it is so hidden that i actually forgot it is even there, so i dont know that it accomplishes that goal.

updating this one is fairly automatic because of the generate:md script that uses remark to update. i dont have a way to fully automate in mind, but i've wanted to investigate a way to fail verification if running the generate script would result in a diff.

open to more feedback on this, but lets address after getting his iteration merged

npm install @repository-settings/app
```

#### Example node.js app
Copy link
Member

Choose a reason for hiding this comment

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

I think it's a good idea, but we can do a follow up for that.

Comment on lines +25 to +60
```js
import {Server, Probot, ProbotOctokit} from 'probot';
import app from '@repository-settings/app';

async function start() {
const log = getLog();
const server = new Server({
Probot: Probot.defaults({
appId: process.env.APP_ID,
privateKey: process.env.PRIVATE_KEY,
secret: process.env.WEBHOOK_SECRET,
Octokit: ProbotOctokit.defaults({
baseUrl: process.env.GH_API
}),
log: log.child({
name: 'repository-settings'
})
}),
log: log.child({
name: 'server'
})
});

process.on('SIGTERM', async () => {
console.log('Received SIGTERM, stopping server!');

await server.stop();
});

await server.load(app);

await server.start();
};

start();
```
Copy link
Member

Choose a reason for hiding this comment

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

I think if you don't need any custom functionality and you want a long-running process, I think there is no code necessary at all, you should be able to do "start": "probot run node_modules/@repository-settings/app/index.js", or maybe @repository-settings/app should expos its own binary like "start": "repository-settings"?

If users need more customization, for example for a serverless environment, then we can point them to https://probot.github.io/docs/development/#run-probot-programmatically and maybe provide an example for a serverless app?

@travi
Copy link
Member Author

travi commented Jan 16, 2024

merging this iteration, with the intention to follow up on the remaining comments in later iterations

@travi travi merged commit 8bc2125 into master Jan 16, 2024
8 checks passed
@travi travi deleted the beta branch January 16, 2024 05:12
Copy link

🎉 This PR is included in version 2.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

None yet

2 participants