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 Package Registry #16510

Merged
merged 184 commits into from
Mar 30, 2022
Merged

Add Package Registry #16510

merged 184 commits into from
Mar 30, 2022

Conversation

KN4CK3R
Copy link
Member

@KN4CK3R KN4CK3R commented Jul 21, 2021

This PR adds support for multiple package registries.

Currently implemented are

Every user/org has an own package registry for all different types of packages.

Screenshots

Package list:
list

Admin page:
grafik

Composer package:
composer

Conan package:
conan

Docker image:
docker1
docker2

Helm Chart:
grafik

Generic package:
generic

NuGet:
nuget

npm:
npm

Maven:
maven

PyPI:
pypi

RubyGems:
rubygems

Instructions for testing: #16510 (comment)

close #15706
close #2316

@KN4CK3R KN4CK3R added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Jul 21, 2021
@KN4CK3R KN4CK3R added this to the 1.16.0 milestone Jul 21, 2021
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 21, 2021
Copy link
Member

@delvh delvh left a comment

Choose a reason for hiding this comment

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

Skimmed through it, what I've seen seems pretty good.
My focus was on Maven as that's what I'm most comfortable with.
No doubt, there will be reviewers that actually read through all 5000 lines of code… 😉

URL string `xml:"url"`
Distribution string `xml:"distribution"`
} `xml:"licenses>license"`
Dependencies []struct {
Copy link
Member

@delvh delvh Jul 21, 2021

Choose a reason for hiding this comment

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

I am not sure whether they are needed, but the Maven Language Server gives me a few more options to pass for each dependency.
They are:

  • optional (String)
  • type (String)
  • exclusions (a set of dependencies)
  • classifier (String)

But I totally understand it if you want that to be part of another PR, the basic functionality already stands.
It wouldn't surprise me, however, if the current implementation fails as soon as one of the advanced features is used, limiting the initial usability. The same is true for the missing fields mentioned below.

Copy link
Member Author

@KN4CK3R KN4CK3R Jul 22, 2021

Choose a reason for hiding this comment

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

I'm no Maven expert but does the client really trust the server informations about the dependencies? I would expect the client downloads the package and checks the local pom file for all needed informations.

Edit: The client downloads the .pom asset with all original informations so all metadata is just used for the UI. I don't know if these are benefitical for a normal user.

Copy link
Member

@delvh delvh Jul 22, 2021

Choose a reason for hiding this comment

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

As I already said, I am also unsure whether they are needed.
Potentially yes, potentially no.
It could, however, be that the package cannot be registered in case advanced properties are used in the pom file. That's what I'm uncertain about.
In the worst case, this will simply have to be added to the data model in another PR, after this PR has been merged and tested thoroughly.

Copy link
Member Author

Choose a reason for hiding this comment

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

The pom file is not generated but the original file is stored. So all metadata is only used for the Gitea UI. Currently I don't think there is a problem.

Version string `json:"version"`
}

type pomStruct struct {

This comment was marked as resolved.

options/locale/locale_en-US.ini Outdated Show resolved Hide resolved
options/locale/locale_en-US.ini Outdated Show resolved Hide resolved
@lunny
Copy link
Member

lunny commented Jul 22, 2021

Since docker is also some kind of package, maybe we should consider what's relation between this one and #14919 . @a1012112796

models/package.go Outdated Show resolved Hide resolved
@lunny
Copy link
Member

lunny commented Jul 22, 2021

Thanks for the fantastic PR! Two small things you can consider to add in this PR. One is add webhooks for packages, PackageUploaded, PackageUpdated, and PackageDeleted. Another is to add a package_log table to record package or files added, updated, deleted.

@lafriks lafriks added the type/changelog Adds the changelog for a new Gitea version label Jul 22, 2021
Copy link
Member

@delvh delvh left a comment

Choose a reason for hiding this comment

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

Holy, I think I've never been sitting so long on a single review as I have now.
About four hours, and I reviewed only the easy 60%...
But before I forget the rest or this PR gets merged, here are a few points I noticed.

docs/content/doc/packages/composer.en-us.md Show resolved Hide resolved
custom/conf/app.example.ini Outdated Show resolved Hide resolved
docs/content/doc/packages/conan.en-us.md Outdated Show resolved Hide resolved
docs/content/doc/packages/generic.en-us.md Show resolved Hide resolved
RegisterTaskFatal("cleanup_packages", &OlderThanConfig{
BaseConfig: BaseConfig{
Enabled: true,
RunAtStart: true,
Copy link
Member

Choose a reason for hiding this comment

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

Wasn't there a setting for this? Is that (now) unused?

Copy link
Member Author

Choose a reason for hiding this comment

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

Are you talking about #19221? If yes, that's already changed.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, that value is simply the default value if the setting is missing, correct?
In that case ignore my comment, I thought it would mean that it would always run at start.

options/locale/locale_en-US.ini Outdated Show resolved Hide resolved
options/locale/locale_en-US.ini Outdated Show resolved Hide resolved
options/locale/locale_en-US.ini Outdated Show resolved Hide resolved
options/locale/locale_en-US.ini Outdated Show resolved Hide resolved
Copy link
Member

@tboerger tboerger left a comment

Choose a reason for hiding this comment

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

Minimal docs typos

docs/content/doc/packages/composer.en-us.md Outdated Show resolved Hide resolved
docs/content/doc/packages/composer.en-us.md Outdated Show resolved Hide resolved
@tboerger
Copy link
Member

Beside the implemented providers it could also make sense to implement the Terraform registry api, Terraform module api and the Terraform remote state api.

@lunny
Copy link
Member

lunny commented Mar 30, 2022

Beside the implemented providers it could also make sense to implement the Terraform registry api, Terraform module api and the Terraform remote state api.

Good idea but let's merge it at first and send other PRs.

@wxiaoguang
Copy link
Contributor

Last call for reviews. If no code change in one day, let's merge.

Co-authored-by: Thomas Boerger <thomas@webhippie.de>
@tboerger
Copy link
Member

@wxiaoguang hopefully my comments will be fixed before the merge ;)

@wxiaoguang
Copy link
Contributor

@wxiaoguang hopefully my comments will be fixed before the merge ;)

Yup, these document fixes have been committed. Anything more to change?

@tboerger
Copy link
Member

Not from my side, looks good. Even if I'm still against bloating the gitea core more and more with features, but this won't change without a plugin system.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Mar 30, 2022

Agree to be more careful about minority features that bloating the core code. Gitea should only introduce features which benefit most users.

Personally speaking, I am not optimistic about Gitea can have a plugin system in near future. Many simple and refactoring PRs just get delayed for long time, even months, I can not imagine how the plugin system could be done in a predictable future. And indeed it seems that a plugin system is not a high priority task at the moment.

@lunny lunny merged commit 1d33234 into go-gitea:main Mar 30, 2022
@lunny
Copy link
Member

lunny commented Mar 30, 2022

Let's merge then send patches. It took so long for this PR.

@wxiaoguang
Copy link
Contributor

3 hours ago Last call for reviews. If no code change in one day, let's merge.

One day comes a little early 😂 but it's not bad, I like it. Let's move on ~~~~~ 🎉 test and patch

zjjhot added a commit to zjjhot/gitea that referenced this pull request Mar 30, 2022
* giteaoffical/main: (31 commits)
  Add Package Registry (go-gitea#16510)
  Show messages for users if the ROOT_URL is wrong, show JavaScript errors (go-gitea#18971)
  [skip ci] Updated translations via Crowdin
  Make git.OpenRepository accept Context (go-gitea#19260)
  Use full output of git show-ref --tags to get tags for PushUpdateAddTag (go-gitea#19235)
  When conflicts have been previously detected ensure that they can be resolved (go-gitea#19247)
  More commit info from API (go-gitea#19252)
  Move some issue methods as functions (go-gitea#19255)
  Move project files into models/project sub package (go-gitea#17704)
  Granular webhook events in editHook (go-gitea#19251)
  Provide configuration to allow camo-media proxying (go-gitea#12802)
  Move init repository related functions to modules (go-gitea#19159)
  Move organization related structs into sub package (go-gitea#18518)
  Refactor repo clone button and repo clone links, fix JS error on empty repo page (go-gitea#19208)
  Show last cron messages on monitor page (go-gitea#19223)
  Allow API to create file on empty repo (go-gitea#19224)
  Use goproxy.io instead of goproxy.cn (go-gitea#19242)
  New cron task: delete old system notices (go-gitea#19219)
  Let web and API routes have different auth methods group (go-gitea#19168)
  Only send webhook events to active system webhooks and only deliver to active hooks (go-gitea#19234)
  ...
@KN4CK3R
Copy link
Member Author

KN4CK3R commented Mar 30, 2022

A little bit rushed at the end but I'm happy it got merged after that long time working on it.

@go-gitea go-gitea locked as resolved and limited conversation to collaborators Mar 31, 2022
@6543
Copy link
Member

6543 commented Mar 31, 2022

locked as new issues should be created & linked to this ... if something pops up

@KN4CK3R KN4CK3R deleted the feature-packages branch April 2, 2022 11:04
@delvh delvh removed the type/changelog Adds the changelog for a new Gitea version label Oct 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. topic/packages type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet