Skip to content

feat: use actions/github-script instead of running node directly#27

Closed
SimenB wants to merge 2 commits into
Pehesi97:gh-action-automatefrom
SimenB:tweaks
Closed

feat: use actions/github-script instead of running node directly#27
SimenB wants to merge 2 commits into
Pehesi97:gh-action-automatefrom
SimenB:tweaks

Conversation

@SimenB

@SimenB SimenB commented Mar 10, 2022

Copy link
Copy Markdown

(this got too big to add as suggestion in the PR, sorry)

We use actions/github-script elsewhere, and it gives us a nice github client we can use instead of using fetch.

Make sure to ignore whitespace to make this reviewable at all: https://github.com/Pehesi97/docker-node/pull/27/files?w=1

Comment thread check-pr-status.mjs
if (data.mergeable_state === 'clean') {
process.exit(0);
console.log(data);
if (data.mergeable_state === 'clean') {

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I wasn't really able to test this one on my branch as I had issues getting the test action to run for some reason... How did you test?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Created a new repo with some simple js scripts and a small test suite, kept polling the GitHub API until the mergeable_state became 'clean'. At creation, it's set to 'unknown'. Then it goes 'unstable' until checks pass, which set that variable to 'clean'

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Repo was public, of course

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Right, but it's never clean due to missing approval, and if approval is added, it might be mergable right away. I think.

Anyways, if automerge doesn't work, we can iterate. Getting the PR to begin with is a great start as it can be merged from mobile, no need to run the script from desktop

Comment thread check-pr-status.mjs Outdated

await setTimeout(retryDelay);
export default async function(github, owner, repository, pull_number) {
const repo = repository.replace(new RegExp(`${owner}/`), '')

@SimenB SimenB Mar 10, 2022

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

might be enough to do const [owner, repo] = repository.split('/')? not sure if it's legal with other / which can mess this up

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Can't think of a one-liner for that. I would split, use the first part and join the other parts.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Seems illegal, so just splitting should be fine:
image

@SimenB SimenB closed this Mar 10, 2022
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