-
-
Notifications
You must be signed in to change notification settings - Fork 17
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
use octokit plugin config from probot #125
base: master
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## master #125 +/- ##
==========================================
+ Coverage 60.13% 61.34% +1.21%
==========================================
Files 2 2
Lines 148 163 +15
Branches 22 26 +4
==========================================
+ Hits 89 100 +11
Misses 45 45
- Partials 14 18 +4
Continue to review full report at Codecov.
|
5ffd82b
to
8198271
Compare
Merging two files does work, but writing a tests for it. Proves difficult and properly needs a merging for the too labels array. |
src/labeler.ts
Outdated
const output = [] as Label[]; | ||
configs | ||
.map(config => { | ||
let labels = config.labels ? config.labels : config; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is needed to verify because the previous config only had an array of labels at the root.
Now if you want to use _extends
and have your own labels
besides those you inherit from _extends
you need to have a key labels
since it would not be valid yaml syntax.
@crazy-max could you take a look? |
@jetersen Sure, keep you in touch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this @jetersen.
Overall looks good to me but got some questions and also looks like there is output issue while printing current labels when _extends
is used. Not sure if this is linked to jest or something else.
README.md
Outdated
- | ||
name: Checkout | ||
uses: actions/checkout@v2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I see everything is now handled through API right? That's why checkout is not necessary anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed.
@@ -0,0 +1,14 @@ | |||
_extends: ghaction-github-labeler:.res/labels.merge1.yml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the context of ghaction-github-labeler:
? Repo? Org?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the context is a bit odd indeed
usually you can simply refer to .github
if you follow the pattern of using the same filename.
however in this case I chose to pick out the file and repo.
You can see the full regex here:
https://github.com/probot/octokit-plugin-config/blob/8443c9a4a2659f2f8661d4d496a62531edeca8ce/src/util/extends-to-get-content-params.ts#L8-L15
also docced at: https://github.com/probot/octokit-plugin-config#the-_extends-key
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the context is repo
on a specific file.
You can also pull from another org
as long as you have read permissions.
This is meant to satisfy a merge test.
@crazy-max if your still interested in this. I'll try and revive it. 😅 Looking at the action marketplace for label syncs this one is the closest to my use case and I think it still would benefit from using github api to load the yaml file and potentially deep merge with an |
This reverts commit 3ab61b3. Throttle is not enabled by default
dc49321
to
85e00ba
Compare
@crazy-max I don't recall the issue you noticed regarding logging. I don't see any issues locally. Seems like you need to approve my workflows, so we could get another test run and see the output. |
Approved
Yes I'm! Will dig this up asap thanks! |
Looking at issues with ci, might need to rebase? |
I forgot to update the yarn lock. I would prefer squash merge tbh. If you want to avoid having to constantly approve my workflow you could add me with a triage permission. I think that should get past the permissions. |
return false; | ||
} | ||
} | ||
|
||
private async updateLabel(label: Label): Promise<boolean> { | ||
try { | ||
const params = { | ||
...github.context.repo, | ||
const parameters = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fyi this are from unicorn eslint. It prefers proper naming of variables.
...github.context.repo | ||
}) | ||
).map(label => { | ||
private static remapLabels(labels: Label[]): Label[] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure this is necessary though it allowed for syntax without unicorn complaining about:
although the method is only used twice so not really sure if cleans up that much.
Running |
@crazy-max gentle reminder :) |
@crazy-max ping |
@crazy-max do you mind trigger the build or at least adding me to triage? Or perhaps I submit another PR with small bit to get permissions. 😅 |
@crazy-max created #171 to solve the permissions issue 😅 Would like to know how GitHub Action sees the test. As you mentioned you notice something off with it. |
Can you rebase with master branch please? There are also changes that should not belong to this PR like changes with eslint, tsconfig, dependencies update or other refactoring. Can we do that in a follow-up instead. I prefer changes in a PR as focused as possible and related to the bug fix or enhancement it provides. If there are multiple changes you would like to make that are not dependent upon each other, consider submitting them as separate pull requests. Thanks! |
@crazy-max Fair enough! |
fixes #9
fixes #3
So probot's config plugin is really cool in that it supports
extends
which gives support for central loading of labels config.The config plugin also allows us to load the config via api.
I implemented tests and chose to use
nock
as a way to avoid messing with the actual GitHub api.You can see the use case here:
https://github.com/Specshell/.github/blob/main/.github/workflows/labels.yml
https://github.com/Specshell/.github/blob/main/.github/labels.yml
https://github.com/Specshell/specshell.software.maven.pom/blob/main/.github/workflows/labels.yml
https://github.com/Specshell/specshell.software.maven.pom/blob/main/.github/labels.yml
in the
.github
repo we want to have central control and using repository dispatch event we can trigger label updates when there are changes to the.github/labels.yml
in.github
repo