[GithubPackages] New github packages bridge #4780
Conversation
Pull request artifacts
last change: Thursday 2026-01-15 18:47:59 |
Mynacol
left a comment
Mynacol
left a comment
There was a problem hiding this comment.
Please polish the bridge a bit more, see my suggestions and comments. Otherwise a good contribution.
Mynacol
left a comment
Mynacol
left a comment
There was a problem hiding this comment.
Thanks for addressing my previous points. Upon revisiting, I found some logic errors in some cases. After addressing these, this bridge will be great ✨.
For additional testing, you can use https://github.com/orgs/curl/packages?repo_name=curl-container.
Empty line required (phpcs.phar check)
Change $plaintext to a string.
Mynacol
left a comment
Mynacol
left a comment
There was a problem hiding this comment.
Thanks, some more cleanups attached.
I hope my reviews are not too annoying. I guess we are mostly done now.
Mynacol
left a comment
Mynacol
left a comment
There was a problem hiding this comment.
Thanks a lot, I found some new minor nitpicks and have some questions to you. We're definitely converging.
| 'NuGet' => 'nuget', | ||
| 'RubyGems' => 'rubygems' | ||
| ], | ||
| 'defaultValue' => 'container', |
There was a problem hiding this comment.
Normally, we set the default value to the most inclusive option. However, here, specifying "All" only works if we don't provide a specific package. So any of the other options is a "safer" option. And among them, I guess containers are the most popular one. What do you think?
| 'defaultValue' => 'container', | |
| 'defaultValue' => 'all', |
| return self::URI | ||
| . urlencode($this->getInput('organization')); | ||
| } else { | ||
| return 'https://github.com/RSS-Bridge/rss-bridge'; |
There was a problem hiding this comment.
Please change this to return an error/null as well. It should not happen anyways (as it's called from functions that already require an organization)?
| $published = ($div->find('relative-time[class=no-wrap]'))[0]; | ||
| $this->items[] = [ | ||
| 'title' => $a->plaintext, | ||
| 'uri' => 'https://github.com' . $a->href, |
There was a problem hiding this comment.
Is something speaking against using defaultLinkTo (shortly after getSimpleHTMLDOM)? That would reduce the chances of bugs.
https://rss-bridge.github.io/rss-bridge/Helper_functions/index.html#defaultlinkto
| 'uri' => 'https://github.com' . $a->href, | |
| 'uri' => $a->href, |
| if (preg_match('/[0-9]+ (hour|hours|day|days|week|weeks|month|months|year|years) ago/', $published->plaintext, $ago)) { | ||
| $this->items[] = [ | ||
| 'title' => $a->plaintext, | ||
| 'uri' => 'https://github.com' . $a->href, |
There was a problem hiding this comment.
Same as above, can probably be simplified in favor of defaultLinkTo.
| 'uri' => 'https://github.com' . $a->href, | |
| 'uri' => $a->href, |
| return parent::getName(); | ||
| } | ||
|
|
||
| public function getUri() |
There was a problem hiding this comment.
I think you have to urlencode the elements (in particular the packagename) to match the behavior of getPackageUri and to suit GitHub.
(e.g. the package curl-container/curl-dev-debian has the URI https://github.com/curl/curl-container/pkgs/container/curl-container%2Fcurl-dev-debian with urlencoded package name and changing %2F to a slash teads to a 404 error)
| { | ||
| if (!empty($this->getInput('organization')) && !empty($this->getInput('repository')) && !empty($this->getInput('packagename'))) { | ||
| if ($this->getInput('packagetype') === 'all') { | ||
| throwClientException('Do not provide package type as "all", when specifying a package name.'); |
There was a problem hiding this comment.
| throwClientException('Do not provide package type as "all", when specifying a package name.'); | |
| throwClientException('Do not provide package type as "all" when specifying a package name.'); |
| . '/packages?ecosystem=' | ||
| . urlencode($this->getInput('packagetype')); | ||
| } else { | ||
| throwClientException('Provide an organization with optional a repository and package name.'); |
There was a problem hiding this comment.
I think my working is shorter and also clear. What do you think?
| throwClientException('Provide an organization with optional a repository and package name.'); | |
| throwClientException('Provide at least an organization.'); |
|
And for the future, the following GitHub organization has sample repos with each package type in the GitHub package registry: https://github.com/orgs/github-packages-examples/repositories |
3 participants
This is a new bridge for github packages. Specify organization/owner, repository, package and package type.