Large rework to enable sending pull requests as user.#10
Large rework to enable sending pull requests as user.#10mithro wants to merge 1 commit intoThe-OpenROAD-Project:mainfrom
Conversation
This is done by using two GitHub Apps;
(a) [OpenROAD Pull Request Sender
](https://github.com/organizations/The-OpenROAD-Project-staging/settings/apps/openroad-pull-request-sender) --
The GitHub App which is given permission by a user to actually send
pull requests as them and then does the request.
It is installed on the `upstream` and `staging` repositories.
It needs the ability to modify pull requests.
(b) [OpenROAD Pull Request Sender Auth Check
](https://github.com/organizations/The-OpenROAD-Project-private/settings/apps/openroad-pr-sender-auth-check) --
The GitHub App which checks that a user has given the OpenROAD Pull
Request Sender the permission to send the pull requests.
It is installed on the `private` repositories.
It only needs the ability to update check status.
The GitHub Apps run using
[Google Cloud Run](https://cloud.google.com/run) (on Google Cloud
Platform) under the `openroad-robot` project.
Signed-off-by: Tim 'mithro' Ansell <tansell@google.com>
| u = (datetime.datetime.utcnow() - self.updated).seconds | ||
| ein = self.created + datetime.timedelta(seconds=self.expires_in) | ||
| rin = self.created + datetime.timedelta(seconds=self.refresh_token_expires_in) | ||
| return f"""\ |
There was a problem hiding this comment.
I think these variable names should not be abbreviated.
From the python style guide.
Function names, variable names, and filenames should be descriptive; eschew abbreviation. In particular, do not use abbreviations that are ambiguous or unfamiliar to readers outside your project, and do not abbreviate by deleting letters within a word.
| rm -rf $(BUILD_DIR) | ||
| mkdir $(BUILD_DIR) | ||
| cp -a $(SRCS) ./$(BUILD_DIR)/ | ||
| find $(BUILD_DIR) -name '*.pem' -delete | ||
| find $(BUILD_DIR) -name '*.pyc' -delete | ||
| find $(BUILD_DIR) -name '.*.sw*' -delete | ||
| find $(BUILD_DIR) -type d -empty -delete |
There was a problem hiding this comment.
Should these steps be extracted into a generic target named clean?
| return f"{self.org}/{self.repo}" | ||
|
|
||
| @classmethod | ||
| def check(cls, s): |
There was a problem hiding this comment.
It's not clear to me what this class method is checking I think a one line description would be helpful here. Also renaming s would be helpful as well.
| assert user, s | ||
| assert org, s | ||
| assert repo, s | ||
| assert pr, s | ||
| assert rev , s |
There was a problem hiding this comment.
If we don't care about the message in the assertion it might just be best to replace s with empty string. I think the s adds confusion.
| return cls.from_token(estate) | ||
|
|
||
| @classmethod | ||
| def from_json(cls, j): |
There was a problem hiding this comment.
Adding a comment on what this is extracting would be helpful. As a user I would like to know what I need to provide to this method to get a useful result out. Also rename j to something more representative of the input like user_get_response.
| c.pop('app') | ||
| if c['external_id'] != EXTERNAL_ID: | ||
| continue | ||
| if c['id'] in (2688818725,): |
There was a problem hiding this comment.
Magic numbers should have a comment explaining where they came from. Ideally this would be a global variable of some kind.
| continue | ||
| existing_checks.append(c) | ||
|
|
||
| t = db.Token.latest(info.user) |
| EXTERNAL_ID = 'pr-auth' | ||
|
|
||
|
|
||
| def auth_check_on_private_pr(info): |
There was a problem hiding this comment.
This method seems to be doing a lot. Giving a one line description with a few sentences underneath to explain intent would help with readability.
| user = flask.request.args.get('user') | ||
|
|
||
| # Get existing token | ||
| t = db.Token.latest(user) |
| now = time.time() | ||
|
|
||
| targets = cls._cache | ||
| if now - cls._cache_updated > 60*5: |
There was a problem hiding this comment.
This number needs to document what units it is in.
|
@mithro is this PR still relevant? |
|
Yes. This makes the pull requests come from the original user rather than the bot. However, I don't recall what the state here was or why it wasn't merged. |
|
I see a number of comments from @QuantamHD that need addressing. I agree the goal is still relevant. |
3 participants
This is done by using two GitHub Apps;
OpenROAD Pull Request Sender -- The GitHub App which is given permission by a user to actually send pull requests as them and then does the request.
upstreamandstagingrepositories.OpenROAD Pull Request Sender Auth Check -- The GitHub App which checks that a user has given the OpenROAD Pull Request Sender the permission to send the pull requests.
privaterepositories.The GitHub Apps run using Google Cloud Run (on Google Cloud Platform) under the
openroad-robotproject.Signed-off-by: Tim 'mithro' Ansell tansell@google.com