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

use octokit plugin config from probot #125

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

jetersen
Copy link

@jetersen jetersen commented Jan 8, 2021

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

@codecov
Copy link

codecov bot commented Jan 8, 2021

Codecov Report

Merging #125 (dc49321) into master (e3f5171) will increase coverage by 1.21%.
The diff coverage is 80.00%.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
src/labeler.ts 59.60% <80.00%> (+1.51%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e3f5171...dc49321. Read the comment docs.

@jetersen jetersen marked this pull request as draft January 8, 2021 19:44
@jetersen
Copy link
Author

jetersen commented Jan 8, 2021

Merging two files does work, but writing a tests for it. Proves difficult and properly needs a merging for the too labels array.

@jetersen jetersen marked this pull request as ready for review January 8, 2021 21:38
src/labeler.ts Outdated
const output = [] as Label[];
configs
.map(config => {
let labels = config.labels ? config.labels : config;
Copy link
Author

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.

@jetersen
Copy link
Author

@crazy-max could you take a look?

@crazy-max
Copy link
Owner

@jetersen Sure, keep you in touch.

Copy link
Owner

@crazy-max crazy-max left a 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 Show resolved Hide resolved
README.md Outdated
Comment on lines 66 to 68
-
name: Checkout
uses: actions/checkout@v2
Copy link
Owner

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?

Copy link
Author

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
Copy link
Owner

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?

Copy link
Author

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

Copy link
Author

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.

@jetersen
Copy link
Author

@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 .github org repo.

@jetersen
Copy link
Author

@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.

@crazy-max
Copy link
Owner

Seems like you need to approve my workflows, so we could get another test run and see the output.

Approved

if your still interested in this.

Yes I'm! Will dig this up asap thanks!

@crazy-max
Copy link
Owner

Looking at issues with ci, might need to rebase?

@jetersen
Copy link
Author

jetersen commented Jul 27, 2022

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.
Although if you would like I would not mind help you maintain the action!

return false;
}
}

private async updateLabel(label: Label): Promise<boolean> {
try {
const params = {
...github.context.repo,
const parameters = {
Copy link
Author

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.

https://github.com/sindresorhus/eslint-plugin-unicorn

...github.context.repo
})
).map(label => {
private static remapLabels(labels: Label[]): Label[] {
Copy link
Author

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:

https://github.com/sindresorhus/eslint-plugin-unicorn/blob/main/docs/rules/no-await-expression-member.md

although the method is only used twice so not really sure if cleans up that much.

@jetersen
Copy link
Author

Running docker buildx bake validate test passes now locally as well. Sorry I was on windows, did not check with Linux 🗡️

@jetersen
Copy link
Author

jetersen commented Jul 29, 2022

@crazy-max gentle reminder :)
I believe this is ready

@jetersen
Copy link
Author

jetersen commented Aug 7, 2022

@crazy-max ping ☺️

@jetersen
Copy link
Author

jetersen commented Aug 11, 2022

@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. 😅

@jetersen
Copy link
Author

@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.

@crazy-max
Copy link
Owner

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!

@jetersen
Copy link
Author

@crazy-max Fair enough!

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.

Support central location for labels Support .github repo label.yml file
2 participants