Add dryrun option and fix some other stuff #30
Conversation
| @@ -14,7 +14,7 @@ | |||
|
|
|||
| import email | |||
| import re | |||
| from typing import List, Optional, Tuple | |||
| from typing import List, Optional, Tuple, BinaryIO | |||
There was a problem hiding this comment.
nit: BinaryIO is unused in this commit.
| except subprocess.CalledProcessError as e: | ||
| logging.warning('Failed to push upstream because %s.', e.output) | ||
| raise | ||
| pass |
There was a problem hiding this comment.
Hmm, I would prefer this DryRunGerritGit to only contain decls + pass
This probably warrants another commit here first to trim down the interface, e.g. rename def push_patch() to def _push_patch()
I think the only thing we want to expose is apply_patchsetup_and_cleanup()?
Then the def would become
class DryRunGerritGit:
def apply_patchsetup_and_cleanup(...):
passThere was a problem hiding this comment.
I've gone ahead and trimmed the interface down as 5adaf15.
So you should just be able to rebase on master.
Might have some merge conflicts (but it felt like those would be unavoidable, even if you made the change yourself)
There was a problem hiding this comment.
I thought about that, but I kind of like it still trying to apply patches locally and doing some other stuff. I found it to provide some useful feedback.
| @@ -40,7 +40,26 @@ def __call__(self, request: PreparedRequest): | |||
| request.prepare_cookies(self.cookie_jar) | |||
| return request | |||
|
|
|||
| class Gerrit(object): | |||
|
|
|||
| class FakeGerrit(object): | |||
There was a problem hiding this comment.
nit: Perhaps DryRunGerrit
This would more symmetric with GerritGit below (assuming we recapitalize to DryRunGerritGit
| @@ -40,7 +40,26 @@ def __call__(self, request: PreparedRequest): | |||
| request.prepare_cookies(self.cookie_jar) | |||
| return request | |||
|
|
|||
| class Gerrit(object): | |||
|
|
|||
| class FakeGerrit(object): | |||
There was a problem hiding this comment.
nit: I'd omit the object, https://google.github.io/styleguide/pyguide.html#39-classes
There was a problem hiding this comment.
Ah yes, I am still stuck on Python 2
| class GerritGit(object): | ||
| def __init__(self, git_dir: str, cookie_jar_path: str, url: str, project: str, branch: str) -> None: | ||
|
|
||
| class DryrunGerritGit(object): |
There was a problem hiding this comment.
nit: perhaps rename to DryRun
Also nit: perhaps drop object, https://google.github.io/styleguide/pyguide.html#39-classes
Ignore several directories that are created when lkml-gerrit-bridge service runs. Change-Id: I757cb9363aa59d45bfd087eae82433a16fc313d3
Add --dryrun flag which cause lkml-gerrit-bridge to run as normal with the exception that it does not upload anything to Gerrit. This is convenient for testing as it makes it faster *and* does not pollute Gerrit. Change-Id: I07e3c5c794a16db8c94e22b1fff7ddae86e0e60e
Handle emails not encoded in utf-8 or ascii. Change-Id: Ia2cbc75bd0f80afada7c6036043d49dcc4e3e049
bjh83
force-pushed
the
add-dryrun-option-and-fix-some-other-stuff
branch
from
fc49061
to
fc61022
Compare
| logging.warning('Failed to push upstream because %s.', e.output) | ||
| raise | ||
|
|
||
| def setup_git_dir(self, clone_depth=1) -> None: |
There was a problem hiding this comment.
I think this wants to be _setup_git_dir
Couple of commits here. I can break them up if you want.
First commit adds some more stuff to .gitignore.
Second commit adds a --dryrun option to service, basically runs everything as normal without uploading to Gerrit.
Last commit fixes email decoding.