Skip to content
This repository has been archived by the owner on Sep 25, 2024. It is now read-only.

Dist-git MR's target branch to match source-git MR's target branch #56

Merged
merged 3 commits into from
Apr 13, 2022

Conversation

majamassarini
Copy link
Member

@majamassarini majamassarini commented Apr 6, 2022

NOTE: tests are failing for the same reason already appeared in #55

Fixes #36

Merge after packit/packit#1555

Related with packit/packit.dev#430

RELEASE NOTES BEGIN

The user can now customize the name of the downstream branch using the downstream_branch_name configuration option.

RELEASE NOTES END

@softwarefactory-project-zuul
Copy link
Contributor

Build failed.

@softwarefactory-project-zuul
Copy link
Contributor

Merge Failed.

This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset.

@softwarefactory-project-zuul
Copy link
Contributor

Build failed.

@majamassarini majamassarini marked this pull request as ready for review April 8, 2022 12:56
@softwarefactory-project-zuul
Copy link
Contributor

Build failed.

@majamassarini majamassarini requested review from jpopelka and csomh April 8, 2022 13:21
Copy link
Member

@jpopelka jpopelka left a comment

Choose a reason for hiding this comment

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

I think we should document the downstream_branch_name somewhere. Given that it's hardly specific thing so far, probably in the hardly/README and we can move it somewhere else (packit.dev) later.

EDIT: Actually, we have source-git specific upstream_ref in the docs as well, so can put downstream_branch_name there as well - mentioning that it's for source-git repos.

@@ -89,6 +89,7 @@ def run(self) -> TaskResults:
Please review the contribution and once you are comfortable with the content,
you should trigger a CI pipeline run via `Pipelines → Run pipeline`."""
dg_mr = self.api.sync_release(
dist_git_branch=self.package_config.downstream_branch_name,
Copy link
Member

Choose a reason for hiding this comment

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

Just to be sure: so if downstream_branch_name is not specified in the config, nothing changes, i.e. it'll still default to the default branch of the dist-git repository, right?

Copy link
Member Author

@majamassarini majamassarini Apr 11, 2022

Choose a reason for hiding this comment

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

Yes, because sync_release is called with dist_git_branch=None

Copy link
Contributor

Choose a reason for hiding this comment

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

This should be changed, so that when donstream_branch_name is not specified use the current source-git branch name as the name of the target branch in dist-git.

@softwarefactory-project-zuul
Copy link
Contributor

Build failed.

@@ -89,6 +89,7 @@ def run(self) -> TaskResults:
Please review the contribution and once you are comfortable with the content,
you should trigger a CI pipeline run via `Pipelines → Run pipeline`."""
dg_mr = self.api.sync_release(
dist_git_branch=self.package_config.downstream_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.

This should be changed, so that when donstream_branch_name is not specified use the current source-git branch name as the name of the target branch in dist-git.

Makefile Outdated
@@ -23,7 +23,7 @@ check:
PYTHONPATH=$(CURDIR) PYTHONDONTWRITEBYTECODE=1 python3 -m pytest --color=$(COLOR) --verbose --showlocals --cov=hardly --cov-report=$(COV_REPORT) $(TEST_TARGET)

test-image: files/recipe-tests.yaml
$(CONTAINER_ENGINE) build --rm -t $(TEST_IMAGE) -f files/Containerfile.tests --build-arg SOURCE_BRANCH=$(SOURCE_BRANCH) .
$(CONTAINER_ENGINE) build --no-cache --rm -t $(TEST_IMAGE) -f files/Containerfile.tests --build-arg SOURCE_BRANCH=$(SOURCE_BRANCH) .
Copy link
Contributor

Choose a reason for hiding this comment

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

This might not be something that is desired in every situation. If you think customization of these options is needed on a regular basis, then we should and a BUILD_OPTIONS variable, or something similar, which could be used in such cases.

softwarefactory-project-zuul bot added a commit to packit/packit that referenced this pull request Apr 11, 2022
Fix/hardly/36

Fixes packit/hardly#36
Merge before packit/hardly#56
RELEASE NOTES BEGIN
A new configuration option downstream_branch_name has been added, which is meant to be used in source-git projects and allow users to customize the name of the branch in dist-git which corresponds to the current source-git branch.
RELEASE NOTES END

Reviewed-by: Jiri Popelka <None>
Reviewed-by: Hunor Csomortáni <[email protected]>
@softwarefactory-project-zuul
Copy link
Contributor

Build failed.

@csomh
Copy link
Contributor

csomh commented Apr 11, 2022

In packit-service, we don't build the test image every time we run the tests, but we pull it from Quay. Shouldn't this be consistent, and done the same way here, too? @jpopelka what do you think?

@jpopelka
Copy link
Member

jpopelka commented Apr 11, 2022

In packit-service we use the pre-built image because building the image takes a long time (it installs all the deps for worker and service and tests).
It has some cons, such as that the tests can fail when the list of dependencies changes, but the pros is the huge speed-up.

In hardly we build the test image on top of worker (until #48) and install only a few libs for tests, which is much faster so it IMHO doesn't make sense to use a pre-built image in this case. But if the build problems don't "go away automagically", then we might, indeed, need to build it in Quay as we do with p-s, because I don't know how to fix them.

@csomh
Copy link
Contributor

csomh commented Apr 12, 2022

Thank you for the explanation @jpopelka. I'll reach out to the softwarefactory folks and ask about this issue.

@softwarefactory-project-zuul
Copy link
Contributor

Build failed.

hardly/handlers/distgit.py Outdated Show resolved Hide resolved
Comment on lines 71 to 73
if dist_git_branch not in self.api.dg.local_project.git_project.get_branches():
msg = f"""
{self.target_repo}:{dist_git_branch} does not exist.
A new branch with this name will be created into the downstream repo.
"""
self.project.get_pr(int(self.mr_identifier)).comment(msg)

return dist_git_branch
Copy link
Contributor

Choose a reason for hiding this comment

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

Tbh, I wouldn't do this check here. If the branch doesn't exist in dist-git, there is nothing meaningful the bot could do: creating it is a release engineering task, it shouldn't be created on the fly. Another solution could be to set downstream_branch_name, but that's also up to the maintainer to decide.

So if the branch doesn't exist in dist-git, opening the PR should simply fail.

@softwarefactory-project-zuul
Copy link
Contributor

Build failed.

@softwarefactory-project-zuul
Copy link
Contributor

Build failed.

@softwarefactory-project-zuul
Copy link
Contributor

Build failed.

When creating a downstream MR use as target branch name
or the same upstream target branch name or a custom one.

Co-authored-by: Hunor Csomortáni <[email protected]>
@softwarefactory-project-zuul
Copy link
Contributor

Build failed.

@majamassarini majamassarini merged commit b2c397a into packit:main Apr 13, 2022
@majamassarini majamassarini deleted the fix/hardly/36 branch May 12, 2022 12:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dist-git MR's target branch to match source-git MR's target branch
3 participants