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

Add Forgejo Issues support #896

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from
Draft

Add Forgejo Issues support #896

wants to merge 11 commits into from

Conversation

mynk8
Copy link
Contributor

@mynk8 mynk8 commented Mar 8, 2025

TODO:

  • Write new tests or update the old ones to cover new functionality.
  • Update or write new documentation in packit/packit.dev.

Fixes #880

RELEASE NOTES BEGIN

ogr now supports Forgejo Issues

  • comment
  • assign labels
  • set title
  • close issue
  • get issues

RELEASE NOTES END

Copy link
Contributor

Copy link
Member

@mfocko mfocko left a comment

Choose a reason for hiding this comment

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

IIUC the docs are inherited from the abstract classes, so we're left with just the pagination and tests

raise IssueTrackerDisabled

parameters: dict[str, Union[str, list[str], bool]] = {
"state": status if status != IssueStatus.open else "open",
Copy link
Member

Choose a reason for hiding this comment

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

I'm not really sure if this produces what you think it does… looking at the definition of IssueStatus, it is an IntEnum, I think you'll find out when you start testing it.

Comment on lines 128 to 158
issues = project.service.api.issue.list_issues(
owner=project.namespace, repo=project.repo, **parameters,
)
return [ForgejoIssue(issue, project) for issue in issues]
Copy link
Member

Choose a reason for hiding this comment

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

This part here is a bit tricky… See packit/packit#2543 (comment) and related notes…

  1. You don't get a full list, cause it is paginated
  2. Once you start fetching everything page-by-page, don't construct a list, implement this as an iterator (I'll be committing similar code for ForgejoProject today, so you should be able to see basic outline of how it looks like)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Totally missed it, will do the changes

)
return ForgejoIssueComment(self, comment)

def _get_all_comments(self) -> list[IssueComment]:
Copy link
Member

Choose a reason for hiding this comment

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

Same here with the pagination

@mynk8
Copy link
Contributor Author

mynk8 commented Mar 16, 2025

So, I wrote all the tests for Forgejo Issues from the Gitlab ones. What's left is to handle pagination, regex tests are not being tested without a pattern for now, will fix later and add docstrings.

Copy link
Contributor

Copy link
Contributor

@mynk8
Copy link
Contributor Author

mynk8 commented Apr 2, 2025

my bad cleaning this up, rebasing

Copy link
Contributor

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.
Warning:
Error merging github.com/packit/ogr for 896,f469d7365a0e6b4e689eda509399cfe7b0333ccf

Copy link
Contributor

Copy link
Contributor

@mynk8 mynk8 marked this pull request as draft April 2, 2025 15:07
@mfocko
Copy link
Member

mfocko commented Apr 3, 2025

Hi, I've pulled in some changes from #893, could you please rebase and see what happens (with the CI)? :)

Also, I've added some utility functions related to the workaround for repeated owner, repo in the API, it would be ideal, if you switched to that from your own “hack”:

  • pagination (note: we have agreed on returning iterables instead of the whole lists, you have implemented it with the lists so far, could you please switch to the iterables? you can also see some of the changes it required in the commits, if you need help, feel free to reach out)
    • function itself
      def paginate(api_call, /, *args, **kwargs):
      api_call = partial(api_call, *args, **kwargs)
      page = 1
      while objects := api_call(page=page):
      yield from objects
      page += 1
    • usage
      def get_tags(self) -> Iterable["GitTag"]:
      return (
      GitTag(
      name=tag.name,
      commit_sha=tag.commit.sha,
      )
      for tag in paginate(self.partial_api(self.api.repo_list_tags))
      )
  • repeated parameters in the API
    • function

      @property
      def api(self) -> RepositoryClient:
      """Returns a `RepositoryClient` from pyforgejo. Helper to save some
      typing.
      """
      return self.service.api.repository
      def partial_api(self, method, /, *args, **kwargs):
      """Returns a partial API call for `ForgejoProject`.
      Injects `owner` and `repo` for the calls to `/repository/` endpoints.
      Args:
      method: Specific method on the Pyforgejo API that is to be wrapped.
      *args: Positional arguments that get injected into every call.
      **kwargs: Keyword-arguments that get injected into every call.
      Returns:
      Callable with pre-injected parameters.
      """
      return partial(
      method,
      *args,
      **kwargs,
      owner=self.namespace,
      repo=self.repo,
      )

      I think you don't have much choice than to copy the partial_api and create another .api property on the ForgejoIssue that would return a reference to the issue API endpoint (rather than the repository one as on the project)

    • usage

      @description.setter
      def description(self, new_description: str) -> None:
      self.partial_api(self.api.repo_edit)(description=new_description)

@mynk8
Copy link
Contributor Author

mynk8 commented Apr 3, 2025

I will implement them as iterators. Thanks for the suggestions; they were much needed.

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 issues
2 participants