feat(treescript): Bug 2015581 Add github create branch action #1341
Conversation
hneiva
force-pushed
the
hneiva/ios-merge-automation
branch
from
133c7fa to
9b3ef08
Compare
hneiva
force-pushed
the
hneiva/ios-merge-automation
branch
from
9b3ef08 to
302b928
Compare
Eijebong
left a comment
Eijebong
left a comment
There was a problem hiding this comment.
A bunch of minor things. Also I would've preferred if this had been split in 2 commits (I'm not even sure submitting the whole thing was intentional), the version bump in xcconfig and the branch creation since they're completely unrelated in code. Neither the commit nor the bug even mentions that version bump semantic change.
| lines = [line for line in contents.splitlines() if line and not line.startswith("#")] | ||
| else: | ||
| # version.xcconfig files should only have a single line with APP_VERSION = 111 | ||
| lines = [line.lstrip("APP_VERSION = ") for line in contents.splitlines()] |
There was a problem hiding this comment.
lstrip here works kinda by miracle and the fact that we don't use any of these characters in the version name. removeprefix would be much more appropriate
| } | ||
|
|
||
| verb = "Would create" if dry_run else "Creating" | ||
| log.debug(f"{verb} {branch_name} on repo {repo}[{repo_id}] at {source_branch}@{source_oid}") |
There was a problem hiding this comment.
| log.debug(f"{verb} {branch_name} on repo {repo}[{repo_id}] at {source_branch}@{source_oid}") | |
| log.debug(f"{verb} {branch_name} on repo {self.repo}[{repo_id}] at {source_branch}@{source_oid}") |
Otherwise you're logging the full response from graphql?
| """Create a new branch in the repository. | ||
|
|
||
| Args: | ||
| branch (str): The name of the new branch to create. |
There was a problem hiding this comment.
| branch (str): The name of the new branch to create. | |
| branch_name (str): The name of the new branch to create. |
| Args: | ||
| branch (str): The name of the new branch to create. | ||
| from_branch (str): The branch to create the new branch from. Uses the | ||
| repository's default branch if unspecified (optional). |
There was a problem hiding this comment.
Missing docs on dry_run, not that it really needs any but that's inconsistent
| return version_info | ||
|
|
||
|
|
||
| # get_version_bump_info {{{1 |
| from treescript.util.task import get_branch, get_create_branch_info, should_push | ||
|
|
||
|
|
||
| def get_branch_name(task: Dict) -> str: |
There was a problem hiding this comment.
| def get_branch_name(task: Dict) -> str: | |
| def get_branch_name(task: Dict) -> Optional[str]: |
Don't think it can be None but then you should use ["branch_name"]. We don't use anything to actually read those type annotations but if we ever do it'd be nice to not have to fix this
|
|
||
|
|
||
| def get_branch_name(task: Dict) -> str: | ||
| """Get the source branch name from a task's create_branch_info. |
There was a problem hiding this comment.
This is the opposite of what this function does isn't it? It's the target branch name, not the source? IMO worth renaming the function to get_target_branch_name also to clear out the confusion with get_branch
| task (Dict): The task definition containing create_branch_info. | ||
|
|
||
| Returns: | ||
| str: The name of the branch to create from, or None if not specified. |
There was a problem hiding this comment.
Same here, this says the opposite of what it does
| repo = (await self._client.execute(str_info_query))["repository"] | ||
|
|
||
| if repo.get("object") is None: | ||
| raise UnknownBranchError(f"branch '{source_branch}' not found in repo!") |
There was a problem hiding this comment.
I wonder if we should also check if the branch already exists and bail early? I'm seeing the "branch creation passed but version bump got 500s" already...
Although there's an argument for saying that if the branch already exists it could've been created by mistake by a human and shouldn't be used and thus failing here is the right thing to do...
2 participants
No description provided.