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

Implement support for Forgejo releases and tags #903

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

olamidepeterojo
Copy link

TODO:

  • Write new tests or update the old ones to cover new functionality.
  • Update doc-strings where appropriate.
  • Update or write new documentation in packit/packit.dev.
  • ‹fill in›

Fixes #881

Related to #866

Merge before/after

RELEASE NOTES BEGIN

Packit now supports automatic ordering of ☕ after all checks pass.

RELEASE NOTES END

Copy link
Contributor

Copy link
Contributor

Copy link
Member

@lbarcziova lbarcziova left a comment

Choose a reason for hiding this comment

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

hello and thanks for the contribution! Mostly the same comments as in #902 apply here, please ideally play with the code a bit locally e.g. by creating your own test repo in https://codeberg.org and once you verify the code works, let us know and we can review it. As for tests, we are using requre for recording the test responses and the contribution guide and the README can help.

name: Optional[str] = None,
tag_name: Optional[str] = None,
) -> "Release":
client = PyforgejoApi(api_url=project.forge_api_url, token=project.token)
Copy link
Member

Choose a reason for hiding this comment

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

instead of initialising the client multiple times, you could create a property, you can get inspired or even use this and this

Comment on lines 20 to 30
@pytest.fixture
def project(service):
repo = os.environ.get("FORGEJO_REPO", "existing_repo_name")
namespace = os.environ.get("FORGEJO_NAMESPACE", "existing_namespace")
project = service.get_project(
repo=repo,
namespace=namespace,
)
if project is None:
pytest.skip(f"Project {namespace}/{repo} not found.")
return project
Copy link
Member

Choose a reason for hiding this comment

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

the code has been updated in the meantime, can you please use the properties project or hello_world_project that were added and run the tests using those?

api_url=self.project.forge_api_url,
token=self.project.token,
)
# Assume the SDK provides a method to download the tarball.
Copy link
Member

Choose a reason for hiding this comment

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

next time please double-check the assumptions, this method doesn't exist 😄

and actually the save_archive method doesn't need to be implemented, it is implemented in the base.py, and checking the usage I don't see any, so we should maybe clean this up even

return f"GitTag(name={self.name}, commit_sha={self.commit_sha})"


class ForgejoGitTag(GitTag):
Copy link
Member

Choose a reason for hiding this comment

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

the classes should inherit from the classes in abstract.py

@olamidepeterojo
Copy link
Author

hello and thanks for the contribution! Mostly the same comments as in #902 apply here, please ideally play with the code a bit locally e.g. by creating your own test repo in https://codeberg.org and once you verify the code works, let us know and we can review it. As for tests, we are using requre for recording the test responses and the contribution guide and the README can help.

Thank you so much for the feedback! 🙏🏽 I’ve gone through your comments and I really appreciate the guidance.

I was able to do some local testing using a test repo on Codeberg as suggested and I’ve verified that the release creation works as expected. I'm looking forward to your review

Copy link
Contributor

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.

Implement support for Forgejo releases and tags
2 participants