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

Update collaborators #92

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

Conversation

tnelling
Copy link
Collaborator

This is completely untested, and doesn't include any hooks to call the update.

@tnelling tnelling added the do-not-merge Used for work in progress PRs label Jan 31, 2019
Copy link
Owner

@jeffkaufman jeffkaufman left a comment

Choose a reason for hiding this comment

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

I like this, though it shouldn't be in bounds.

It's ok to run it and test it. When it's working we can add it?

Note that everything you're doing here you can do running the script locally.

util.py Outdated Show resolved Hide resolved
collaborators.py Outdated
util.request_put('%s/%s' % (_collaborators_url(repo), user))

def _collaborators_url(repo):
return 'https://www.jefftk.com/nomic-github/repos/%s/collaborators' % repo
Copy link
Owner

Choose a reason for hiding this comment

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

hmm, I should have a whitelist of what kind of requests to proxy

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I assume this won't actually work until you set that up? Or is the issue that you're currently letting anything through?

Copy link
Owner

Choose a reason for hiding this comment

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

Before 528a406 it was letting through everything under https://api.github.com/repos/jeffkaufman/nomic but now it's restricted to https://api.github.com/repos/jeffkaufman/nomic/pulls Still, this would allow someone to PUT to https://api.github.com/repos/jeffkaufman/nomic/pulls/N/merge and since I'm an admin I think they might be able to merge without Travis passing. So in 83f90b7 I limited it to only GET requests.

If we decide to poke a hole through for collaborators we can, but we should be careful about it. For example, I don't know what would happen if you removed me!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm guessing it would fail, though I'm not inclined to test it. We could have it skip over you, or whoever is the repo owner. That doesn't address the broader proxy issue, though, just the code in this PR.

Copy link
Owner

Choose a reason for hiding this comment

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

Right, but since anyone can poke the proxy we need the proxy to do validation.

Copy link
Owner

Choose a reason for hiding this comment

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

The easiest thing to do might be to stand up your own nginx? Then I don't have to figure out how to do the validation...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe I'm misunderstanding something, but how would that give me permissions? I don't have your GitHub credentials to use. Are you saying that I could hit the proxy without you needing to update the whitelist?

Copy link
Owner

Choose a reason for hiding this comment

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

The idea is you'd use your own github credentials and play around with the tnelling/nomic repo. But I'm not sure there's a good way to open this up? Letting anyone add/remove collaborators doesn't seem great, and that's what the proxy would have to do.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, of course. Might generate some annoying email traffic, but would work otherwise.

Could the proxy only let traffic through to collaborators from Travis (travis-ci.com/jeffkaufman/nomic, even)? And could we consider requests to collaborators to be out-of-bounds?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If there was a way to restrict it to only builds run on master, I'd suggest that, but I can't see how the proxy could possibly tell the difference.

collaborators.py Outdated Show resolved Hide resolved
@jeffkaufman
Copy link
Owner

Overall I'm not sure this is worth it: it's just not that hard to handle manually.

@tnelling
Copy link
Collaborator Author

I think this is more valuable as a long-term thing, to get the baseline repo closer to something that anyone could fork and start their own game without needing to know how to do as much administration as you've been doing. Granted, right now there's hard-coded jefftk stuff all over the place (not just in this PR).

@jeffkaufman
Copy link
Owner

anyone could fork and start their own game without needing to know how to do as much administration as you've been doing

Most of the work is setting up the proxy and travis. When we're done (or if someone asks) I'm happy to work on getting that better documented. Those are harder to automate (or, automate beyond how much they're already automated).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge Used for work in progress PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants