-
Notifications
You must be signed in to change notification settings - Fork 61
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
Add: test action install pyosmeta #184
Conversation
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.
As I saw my initial reply to you on Slack was out of date, since it seems you fixed the problem you'd asked and a couple other things, I figured I'd update my remaining feedback and turn it into a proper review, for much more convenient digestion, and add a few more things I noticed. Hopefully this is helpful!
Beyond the specific comments, while I'm not totally sure given the lack of a high-level description of the exact purpose here, it seems to me that if you're just trying to update a list of contributors after each change, it seems possible that you might want to rethink you're approach a bit, since there are a couple ways to do this that would be simpler and more efficient, and avoid unnecessary klunkyness and noise.
Does the file have to be checked in to source control? If not, it seems a simpler approach would just be to add a step to your build-site
workflow that generates the file on demand (if its expensive, then you could limit it to push
triggers only), which is simpler, cleaner, faster and less work than making a whole separate PR. If it's still too expensive to regenerate from scratch all the time, you could cache it using the cache
action to cache the file between pushes, so it works just like it does currently, without the overhead of running a whole separate workflow to submit a PR and merge it.
Alternatively, if it has to be checked in, you could add a job that regenerates and commits it automatically, then you could run a job that on-demand (e.g. via a comment command) or even automatically commits the re-generated file. You can use this workflow I built that does the same thing for CPython; just replace the regenerate configure step with "Install pyosmeta and run update contribs", replace the commit step with yours and it should work (we use it in production on the docrepr repo already).
After further discussion, it seems what you're actually trying to do is periodically update this file from external data sources, and thus this overall approach does indeed seem generally appropriate. I'll add a few additional suggestions to implement that. Sorry for the confusion!
Best of luck, and happy to help with further questions/issues!
name: Update Contributors | ||
|
||
on: | ||
pull_request: |
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.
pull_request: |
You'll want to remove this trigger before actually merging the PR, presumably
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.
oh yes - i just have this here so i can see how this runs when i update it. i do want to move this to a cron job once it works (and i'll rebase away all of my blblblblblb commits too 😆 i just haven't gotten this to actually create a pr yet is the problem.
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.
Seems like this wasn't actually removed before merging, so things might go haywire as soon as people push to other PRs.
Pro tip: The "Require conversations to be resolved before merging" branch protection rule is a good way to catch these sorts of things that can be easy to miss; we use it on a lot of our Python and Spyder repos.
echo "$PWD" | ||
git branch |
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.
echo "$PWD" | |
git branch |
The echo $PWD
is already run just below (with nothing that could change the CWD in between), and you removed the branch creation step before this (and create-pull-request
will print it anyway), so it seems like these are both vestigial. And just in case you do need it, it seems the next step would be a more logical place for it.
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.
ok! this was added because i didn't understand why i wasn't seeing changes / a pr from this action. i am still a bit confused about how the branch part of this action works.
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.
Hopefully the explanation I DM'ed was helpful, but if not happy to explain further or hop on a quick call.
"https://raw.githubusercontent.com/pyOpenSci/python-package-guide/main/.all-contributorsrc", | ||
"https://raw.githubusercontent.com/pyOpenSci/software-peer-review/main/.all-contributorsrc", | ||
"https://raw.githubusercontent.com/pyOpenSci/pyopensci.github.io/main/.all-contributorsrc", | ||
"https://raw.githubusercontent.com/pyOpenSci/software-review/main/.all-contributorsrc", | ||
"https://raw.githubusercontent.com/pyOpenSci/update-web-metadata/main/.all-contributorsrc", |
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.
Minor side comment, but is there a reason that all these files are missing their extensions? A lot of helpful tooling and infra (pre-commit, editors, many things on GitHub, etc.) doesn't work right if the files don't have the correct extensions.
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.
good point - this is out of my control - https://github.com/pyOpenSci/pyopensci.github.io/blob/main/.all-contributorsrc the all contributors bot that i use to track contribs across our repos creates this file. so for whatever reason it doesn't have an extension. i could look into whether this has changed since i installed it but i didn't pick that file name. (or atleast i dont think i did?)
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.
Oh, I see—this is indeed out of your control, then!
I'd mark this as resolved, except I can't as I'm not an org member. For some strange reason, GitHub's permissions are such that as a non-org-member I cannot mark my own review comments as resolved, but I can mark other people's review comments as resolved on my own PRs, which is exactly backward to what would make sense.
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
ok I am taking a break: i think this is generally working BUT it's failing on the pull request step i think because it doesn't have correct permissions. i created a fine grained token that should have access to this repo with pr and write access. so:
|
i have a new theory - my token is actually for pyos but this pr comes from my fork of the pyos website repo. i wonder what happens if i merge this into main (after cleaning up commits) |
ok i am going to merge this to test. and see if it runs given the scope of the current token is specific and won't work on my fork. |
@all-contributors please add @CAM-Gerlach for code, design, review |
I've put up a pull request to add @CAM-Gerlach! 🎉 |
*hypothesis 🙂 As the relevant log output shows, it was correctly attempting to push the branch to the upstream repo, not your fork:
The problem here is likely that since the job is running from a pull request, it's picking up the repository secrets of your fork (or none at all) and not the usptream repo. If that was the case, then anyone who knew the name of the secret in question (e.g. through being used in a workflow) could easily steal it by submitting a PR with a modified workflow that prints/upload the secret's contents. Therefore, without the PAT and only So, in order to test that a PR is actually created as expected, you have to merge this regardless. However, you shouldn't need to create your own personal access token at all for this—it's extra complexity, risk and bus factor (since its tied to your specific personal account, rather than the repo/org). Instead, you should be able to just give the default permissions:
pull-requests: write |
@CAM-Gerlach if you are open to it - i'd welcome a PR to fix this action by adjusting the token privileges as you suggest and turning it into a cron job that runs every 2 weeks rather than something that runs on each pr. If you have time for this i'd love the help. |
Sure, I can't promise it tomorrow given my long backlog at the moment, but I can give it a shot within the next week. Thanks! |
oh gosh - no worries - if you are too busy no worries! i know you have a LOT on your plate!! |
The goal of this PR is to automate the process of updating our contributor list (contributors across all repos and peer review steps) on our website here. Currently i've been randomly updating it manually using some code i am developing in the pyos-meta package.
this process will
the goal here will be to create a cron job that runs periodically - maybe every 2 weeks - and updates the contrib list.