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

feat: enable multiple target branches for batching MRs (#267) #323

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sg70
Copy link
Contributor

@sg70 sg70 commented Oct 1, 2021

In order to support batch jobs in parallel, the batch branches
need to be tied to the target branch name.

Signed-off-by: Sascha Binckly [email protected]

@sg70
Copy link
Contributor Author

sg70 commented Oct 1, 2021

@qqshfox we have used this custom version for a couple of month without any issue. It would be nice to have this merged into the project.

@@ -20,19 +20,21 @@ class BatchMergeJob(MergeJob):
def __init__(self, *, api, user, project, repo, options, merge_requests):
super().__init__(api=api, user=user, project=project, repo=repo, options=options)
self._merge_requests = merge_requests
# In order to run unit tests as standalone, initialize _batch_branch_name
self._batch_branch_name = BatchMergeJob.BATCH_BRANCH_NAME
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you elaborate why this is needed please? Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The unit test do expect the fix name of the batch branch. The change does introduce a dynamic name of the batch branch depending on the target branch. Line 24 was a shortcut to have green tests.

Anyhow, I reworked the class. Class methods which do use the name of the batch branch did get an additional parameter (batch_branch_name). IMO this does make the code more readable.
At the same time the initialization of class attribute is not nessary anymore.

@qqshfox would you approve this PR now?

@sg70 sg70 force-pushed the issue-267-enable-multi-branch-for-batching-mrs branch 8 times, most recently from a0e44b7 to 3d8c496 Compare March 31, 2022 16:40
In order to support batch jobs in parallel, the batch branches
need to be tied to the target branch name.

Signed-off-by: Sascha Binckly <[email protected]>
@sg70 sg70 force-pushed the issue-267-enable-multi-branch-for-batching-mrs branch from 3d8c496 to d9354ac Compare March 31, 2022 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants