Skip to content

Tag app authors in PRs to their apps #3935

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

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

Conversation

bobrippling
Copy link
Collaborator

@bobrippling bobrippling commented Jul 11, 2025

As seen in #3932, it can take a bit of research to find the author of an app. This PR adds a CODEOWNERS file so owners can add themselves to be notified of PRs which might need their review.

I've added a few people who I could programatically glean from the git history

@bobrippling
Copy link
Collaborator Author

@gfwilliams @thyttan what do you think?

@bobrippling
Copy link
Collaborator Author

Seems it can only include people with write access. Maybe we have a script parse this file to tag those without write access?

@gfwilliams
Copy link
Member

In theory it's good to have a common place, but having to look apps up in one text file seems almost as hard as clicking the GitHub icon next to an app and looking at the history?

Could this be a new field in the metadata file? Npm uses "author". The app loader could easily pick it up then?

@bobrippling
Copy link
Collaborator Author

Yes, the problem with git history is there's a lot of commits by non-owners of the app, so a lookup isn't always trivial and my goal is to see if we can get a script to automatically tag the owner. An author field sounds good, more localised too!

We could try out this setup I've changed to

@bobrippling bobrippling changed the title Add CODEOWNERS Tag app owners in PRs to their apps Jul 12, 2025
@thyttan
Copy link
Collaborator

thyttan commented Jul 12, 2025

In my head the word owner rings a little bad in this open source context. Author sounds better to me. But maybe that's just me bikeshedding 🙃

@bobrippling bobrippling changed the title Tag app owners in PRs to their apps Tag app authors in PRs to their apps Jul 13, 2025
@bobrippling
Copy link
Collaborator Author

Agreed!

@gfwilliams
Copy link
Member

This looks good - looks like the mention-app-authors thing fails though - maybe due to excessive requests? https://github.com/espruino/BangleApps/actions/runs/16239627985/job/45854268885?pr=3935

Might be easier to just run it manually and add it to the PR that way?

Please can you also add a mention in the README on the format? https://github.com/espruino/BangleApps?tab=readme-ov-file#metadatajson-format

And the tutorial is going to need changing too: https://github.com/espruino/EspruinoDocs/blob/master/tutorials/Bangle.js%20App%20Loader.md#adding-your-app

You're probably not going to like this, but do you think you'd be able to insert the author field in a different place in the JSON? Maybe after version? Having it right at the bottom looks a bit odd, and as we're doing it automatically it's probably not that bad compared to fixing it on almost 800 apps afterwards

@bobrippling
Copy link
Collaborator Author

Yeah I need to debug that 403, I'll get it working then sort out the surrounding bits - good catches

@bobrippling bobrippling force-pushed the feat/codeowners branch 6 times, most recently from 22e8fe8 to 99767f0 Compare July 23, 2025 17:45
@bobrippling
Copy link
Collaborator Author

@gfwilliams seems like the 403's a permission error. I can't see the repo settings, is it possible that actions aren't setup with permission to post comments?

Resource not accessible by integration

@gfwilliams
Copy link
Member

I'm not quite sure what I should be looking for, but as far as I can see the main permissions are granted - this looks ok?

image

Looking at the PR it seems it's been able to do some of the files, so either it's hit a rate limit (which seems likely?) or it's trying to access a URL that's not there - like maybe . or .. or something that get included?

... but maybe it's not a big deal - I feel like you could just do this at your end and include it in the PR without the runner that'd be fine?

@bobrippling
Copy link
Collaborator Author

Dang, ok i'll investigate when i get chance next - I'm currently running the script but it's a faff and my aim here is to remove the manual steps entirely

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.

3 participants