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 uploading packages through the Web UI #26960

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

Conversation

JakobDev
Copy link
Contributor

@JakobDev JakobDev commented Sep 7, 2023

At the Moment you can upload Packages only through the API. So if you want to e.g. upload a Debian package that you have build, you need to lookup the command and maybe you also need to create a Token. This PR adds a easier way: Now you can upload packages through the Web UI with just a few clicks! This will of course not work with all package types. You can't expect to upload e.g. a Container Image this way. This PR aims at those Packages types, than can be uploaded using curl.

Currently supported types:

  • Alpine
  • Debian
  • Generic
  • RPM

To-do:

  • Support more types
  • Improve UI
  • Make UI translatable
  • Find bugs

Screenshots:
grafik

cc @KN4CK3R

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Sep 7, 2023
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 7, 2023
@silverwind
Copy link
Member

  • Filename seems to conflict with uploaded file. Do we require a certain naming pattern?
  • The file input should look like the text inputs, e.g. label before, a break and same width as the other inputs

@JakobDev
Copy link
Contributor Author

JakobDev commented Sep 7, 2023

Filename seems to conflict with uploaded file. Do we require a certain naming pattern?

The Filename is only used by generic Packages. It's the filename under which the package is published later. I though it was a good idea to add this instead of using the name from the uploaded file, so nobody ends up with something like package_test4.zip. Maybe it would be a good idea to use something like "(leave empty to use the file name from the uploaded file)" for this field.

The file input should look like the text inputs, e.g. label before, a break and same width as the other inputs

I will do that.

@silverwind
Copy link
Member

silverwind commented Sep 7, 2023

What do those package filenames look like? I assume they are different per type of package? Maybe we can keep things simple and just auto-generate a filename from name and version only?

@silverwind
Copy link
Member

Oh and the input placeholders in UI should show sample values, e.g. 1.2.3 for version for example.

@JakobDev
Copy link
Contributor Author

JakobDev commented Sep 7, 2023

What do those package filenames look like?

grafik

Hallo.txt is the filename in this case.

I assume they are different per type of package?

No. As I said, they only exists for generic packages.

Maybe we can keep things simple and just auto-generate a filename from name and version only?

That sounds more complex and not simpler. The Version filed exists (as far as I know) also only for generic packages. For other Formats, the Version is extracted from the package itself.

Oh and the input placeholders in UI should show sample values, e.g. 1.2.3 for version for example.

You can use whatever you want as version. There is no need to use somtehing like 1.0. You can also use MyCoolNewVersion as version.

Here are Screenshots how it currently looks:
grafik
grafik

routers/web/shared/packages/upload.go Outdated Show resolved Hide resolved
routers/web/web.go Outdated Show resolved Hide resolved
@pull-request-size pull-request-size bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 7, 2023
@KN4CK3R
Copy link
Member

KN4CK3R commented Sep 7, 2023

I assume they are different per type of package?

No. As I said, they only exists for generic packages.

Just to note, there is filename related logic in other package types too (Conan, Maven, NuGet, ...)

@JakobDev
Copy link
Contributor Author

JakobDev commented Sep 7, 2023

Just to note, there is filename related logic in other package types too

Thanks. I will see that when I add those types. But for this PR, I think it's better to focus on those packages that can be uploaded with a simple PUT request.

I also added support for RPM packages now.

@silverwind
Copy link
Member

UI looks good, but I would replace "The Package:" with "File:"

@silverwind silverwind added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Sep 7, 2023
@github-actions github-actions bot added modifies/api This PR adds API routes or modifies them and removed topic/packages labels Sep 12, 2023
@JakobDev
Copy link
Contributor Author

I had moved to upload code now to the specific service of each package rather than bundling all in one upload service

techknowlogick pushed a commit that referenced this pull request Sep 12, 2023
@pull-request-size pull-request-size bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 12, 2023
@JakobDev
Copy link
Contributor Author

I have now added support for Alpine Packages. I also consider this PR now as ready. There are package types missing, but they can be added in a later PR, as I don't want to grow his PR too big. I think those 4 package types are a good start.

@JakobDev JakobDev marked this pull request as ready for review September 12, 2023 09:56
@JakobDev JakobDev requested a review from KN4CK3R September 12, 2023 09:56
@puni9869
Copy link
Member

Is it ready for review?

@JakobDev
Copy link
Contributor Author

Yes, it is ready to review

@puni9869
Copy link
Member

Can you add some unit test cases, if applicable?

@JakobDev
Copy link
Contributor Author

Can you add some unit test cases, if applicable?

I don't know how to do this. We don't have something like Selenium to write real frontend tests (I think this should be introduced). The only thing I could do is making a POST request to the form endpoints. But everything I found in the Gitea test suite that does this for the frontend (Search for GetCSRF) uses only strings. But we have a multipart form with file uplo0ad here. Testing this would most likely require some changes in the test suite. Luckily the package upload code itself is already tested. So the only backend files that are not tested are those in routers/web.

@puni9869
Copy link
Member

puni9869 commented Sep 19, 2023

no worries

with playwright we can achieve that

@JakobDev
Copy link
Contributor Author

Its looks like playwright is not really used besides some examples

routers/web/shared/packages/upload.go Outdated Show resolved Hide resolved
services/packages/alpine/upload.go Outdated Show resolved Hide resolved
services/forms/package_form.go Outdated Show resolved Hide resolved
@JakobDev JakobDev requested a review from KN4CK3R October 7, 2023 17:05
@ghost ghost mentioned this pull request Oct 12, 2023
@ghost
Copy link

ghost commented Nov 14, 2023

@JakobDev

I have function that might help to automatically get a response code in PR, it it is not finished, i am not sure about response codes.

// Automatically get HTTP status from error.
func StatusFromError(err error) int {
	switch {
	case errors.Is(err, ErrNotExist):
		return http.StatusNotFound
	case errors.Is(err, ErrAlreadyExist) || errors.Is(err, ErrLimitExceeded):
		return http.StatusConflict
	case errors.Is(err, ErrPayloadTooLarge):
		return http.StatusRequestEntityTooLarge
	case errors.Is(err, ErrInvalidArgument) || errors.Is(err, io.EOF):
		return http.StatusBadRequest
	case errors.Is(err, ErrPermissionDenied):
		return http.StatusForbidden
	}
	return http.StatusInternalServerError
}

It might be possible to use this function to check, upload error is caused by user, and which type is it. Should be possible not to return IsUserError value and not to make manual response code assignment (response code can be used to check, if error is caused by user, if reponse code is not InternalServerError).

@ExplodingDragon
Copy link
Contributor

Any new update on this @JakobDev ?

@JWT007
Copy link

JWT007 commented Oct 18, 2024

@JakobDev would really love to see this feature merged ... been a year since it got any love though :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. modifies/api This PR adds API routes or modifies them size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. topic/packages type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants