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

feat(generics): allow upload of build assets as generic packages #174 #343

Conversation

JonasSchubert
Copy link
Contributor

@JonasSchubert JonasSchubert commented Mar 22, 2022

Progress of #283 which I closed due to missing time on my side

Gitlab allows to add generic packages as assets attached to a release. #174 This is a suggestion to define a list of generics to be published into the generics repository of a project and then to be used as assets for the release.

@fgreinacher fgreinacher self-requested a review March 25, 2022 19:41
README.md Outdated Show resolved Hide resolved
@fgreinacher fgreinacher mentioned this pull request Apr 5, 2022
@JonasSchubert JonasSchubert force-pushed the feat/allow-generic-packages-as-assets branch 2 times, most recently from 8f5fbb8 to d6716c1 Compare April 6, 2022 19:15
@fgreinacher
Copy link
Contributor

@JonasSchubert Could you check if it makes sense to pull the changes from #322 in this MR? It seems to touch the very same code paths.

@JonasSchubert
Copy link
Contributor Author

Will check

@JonasSchubert
Copy link
Contributor Author

@JonasSchubert Could you check if it makes sense to pull the changes from #322 in this MR? It seems to touch the very same code paths.

I would tackle this in a separate PR. I checked the code and in my opinion it needs major adjustments regarding assets handling.

README.md Outdated Show resolved Hide resolved
lib/publish.js Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@fgreinacher fgreinacher left a comment

Choose a reason for hiding this comment

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

Again just a small documentation suggestion.

@JonasSchubert
Copy link
Contributor Author

I would wait for #322 and merge afterwards.

@fgreinacher
Copy link
Contributor

I would wait for #322 and merge afterwards.

Fine for me! Ping me again in case you change your mind :)

@JonasSchubert JonasSchubert force-pushed the feat/allow-generic-packages-as-assets branch 2 times, most recently from e404962 to 2050fa9 Compare May 22, 2022 14:06
@mfoti
Copy link

mfoti commented Jun 23, 2022

any update on this?

@bgoareguer
Copy link

@fgreinacher @JonasSchubert #322 has been merged. Can you please merge this PR?

@fgreinacher
Copy link
Contributor

Happy to merge once conflicts are resolved. @JonasSchubert are you still willing to work on this?

@JonasSchubert
Copy link
Contributor Author

Sorry everyone! I lost track of some stuff...
Fixed the conflicts and ready to merge. Will keep an eye in case any issues should occur

@JonasSchubert JonasSchubert force-pushed the feat/allow-generic-packages-as-assets branch from fa5680c to cb7ce48 Compare January 28, 2023 16:45
@JonasSchubert
Copy link
Contributor Author

@fgreinacher Rebased to v10 and fixed lint errors. Should be fine now.

Copy link
Contributor

@fgreinacher fgreinacher left a comment

Choose a reason for hiding this comment

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

LGTM, let's get this in finally :)

@fgreinacher fgreinacher merged commit 0067996 into semantic-release:master Jan 30, 2023
@github-actions
Copy link

🎉 This PR is included in version 10.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@mfoti
Copy link

mfoti commented Feb 14, 2023

Thanks for the work to all, and for merging this.

I've tried this feature and I guess there are two bugs:

  1. the encoded url should end with the file name, not the label here: https://github.com/semantic-release/gitlab/blob/master/lib/publish.js#L91 as in documentation (https://docs.gitlab.com/ee/user/packages/generic_packages/#publish-a-package-file)

The url should be like this:

PUT /projects/:id/packages/generic/:package_name/:package_version/:file_name?status=:status

This code should fix: /projects/${encodedRepoId}/packages/generic/release/${encodedGitTag}/${encodedFileName}

I got this error creating labels like this: "MyApp x86 (.dmg)"

  1. The url should not be taken from the response body (https://github.com/semantic-release/gitlab/blob/master/lib/publish.js#L105) but should be the value of uploadEndpoint.
    Actually when it does a release, it publish the package with an url like this:
"links": [
      {
        "name": "MyApp-v2.9-x64.dmg",
        "url": "https://storage.googleapis.com/gitlab-gprd-package-repo/25/87/2587b9f64886c595ba69e1e67434ca7c69a120154ad2d3d5a4a10ddca78a978e/packages/12549696/files/68634567/MyApp-v2.9-x64.dmg?GoogleAccessId=gitlab-object-storage-prd@gitlab-production.iam.gserviceaccount.com&Signature=M03DQehdSKyLvpdTBKAuxiveI4yANOMgKcFs5etCp%2FTwGoj%2BoQy%2FRpoiSPGd%0Ak6leI4uVtOv55w%2BZa764o%2FENpOQ2eFTOfIr6H45Hyi5QupPArB0OL0164Wul%0AgmhqQtcP%2FZ%2FQEVm%2F40Vq%2Fa50MPoPgOa4%2BRt%2FDS9CjWVPXz2PvKz9Zzy0P114%0AXiEyaMxSCQmPcai6%2BYFetwTI%2Fi%2Fm4HaSkOqG%2BLhHd%2BswJDNZNBucZo7gf2fg%0APbGQkJ3wQxzVY%2BukhxjMm%2BjXw8kR%2F%2FPsu8F810gjLzMsnCYNt2O5pwpCY5fr%0AzDP%2BQ1YosqKiTuBhWR6kPUUh43j6mdKcjmbGv7vJcA%3D%3D&Expires=1676411480",
        "link_type": "package"
      }

After few minutes the package is no longer available, loading the package url I got this response:

<Error>
<Code>ExpiredToken</Code>
<Message>The provided token has expired.</Message>
<Details>Request signature expired at: 2023-02-14T21:51:20+00:00</Details>
</Error>

I guess that this can fix the bug: assetsList.push({ label, alt: "release", uploadEndpoint, type: "package", filepath, target });

I can do the PRs if needed

@JonasSchubert
Copy link
Contributor Author

JonasSchubert commented Feb 15, 2023

Hi @mfoti and thanks for your report.

  1. the encoded url should end with the file name, not the label here: https://github.com/semantic-release/gitlab/blob/master/lib/publish.js#L91 as in documentation (https://docs.gitlab.com/ee/user/packages/generic_packages/#publish-a-package-file)

The url should be like this:

PUT /projects/:id/packages/generic/:package_name/:package_version/:file_name?status=:status

This code should fix: /projects/${encodedRepoId}/packages/generic/release/${encodedGitTag}/${encodedFileName}

I got this error creating labels like this: "MyApp x86 (.dmg)"

As the README describes, the label represents the file name. IMO the current implementation is here correct.

  1. The url should not be taken from the response body (https://github.com/semantic-release/gitlab/blob/master/lib/publish.js#L105) but should be the value of uploadEndpoint.
    Actually when it does a release, it publish the package with an url like this:
"links": [
      {
        "name": "MyApp-v2.9-x64.dmg",
        "url": "https://storage.googleapis.com/gitlab-gprd-package-repo/25/87/2587b9f64886c595ba69e1e67434ca7c69a120154ad2d3d5a4a10ddca78a978e/packages/12549696/files/68634567/MyApp-v2.9-x64.dmg?GoogleAccessId=gitlab-object-storage-prd@gitlab-production.iam.gserviceaccount.com&Signature=M03DQehdSKyLvpdTBKAuxiveI4yANOMgKcFs5etCp%2FTwGoj%2BoQy%2FRpoiSPGd%0Ak6leI4uVtOv55w%2BZa764o%2FENpOQ2eFTOfIr6H45Hyi5QupPArB0OL0164Wul%0AgmhqQtcP%2FZ%2FQEVm%2F40Vq%2Fa50MPoPgOa4%2BRt%2FDS9CjWVPXz2PvKz9Zzy0P114%0AXiEyaMxSCQmPcai6%2BYFetwTI%2Fi%2Fm4HaSkOqG%2BLhHd%2BswJDNZNBucZo7gf2fg%0APbGQkJ3wQxzVY%2BukhxjMm%2BjXw8kR%2F%2FPsu8F810gjLzMsnCYNt2O5pwpCY5fr%0AzDP%2BQ1YosqKiTuBhWR6kPUUh43j6mdKcjmbGv7vJcA%3D%3D&Expires=1676411480",
        "link_type": "package"
      }

After few minutes the package is no longer available, loading the package url I got this response:

<Error>
<Code>ExpiredToken</Code>
<Message>The provided token has expired.</Message>
<Details>Request signature expired at: 2023-02-14T21:51:20+00:00</Details>
</Error>

I guess that this can fix the bug: assetsList.push({ label, alt: "release", uploadEndpoint, type: "package", filepath, target });

This is a little bit more tricky then 1..

For me it looks like you use Google Storage to store some assets and packages and this provider returns an URL with an expiration date. Not sure whether this is for the URL only or also for the file.
Interesting would be what happens if we use the uploadEndpoint? Even if we use this URL isn't the returned URL still active in the background and by this probably causing the same issue?
Would it be possible for you to test this in your project?

BTW the currently used URL is the one provided by the API call response according to the docs.
With this said, the docs also highlight how to download a generic package.

But:

If multiple packages have the same name, version, and filename, then the most recent one is retrieved.

With the current approach we have direct access to the uploaded file without worrying about this drawback, which we might loose using the Gitlab URL.

@mfoti what are your thoughts on this?

@mfoti
Copy link

mfoti commented Feb 15, 2023

As the README describes, the label represents the file name. IMO the current implementation is here correct.

I guess defaults to the filename, but allow to put a string as package description, as in my case: "MacOS x64 (.dmg)"

BTW I can confirm that using the filename as label works:

       // This works
       {
          label: `MyApp-${nextRelease.version}-x64.dmg`,
          path: `release/build/MyApp-${nextRelease.version}-x64.dmg`,
          type: "image",
          target: "generic_package"
        },

But if you use a different label it doesn't

       // This doesn't works
       {
          label: 'MacOS x64 (.dmg)',
          path: `release/build/MyApp-${nextRelease.version}-x64.dmg`,
          type: "image",
          target: "generic_package"
        },

I guess to avoid errors (release is correctly generated with a label different of the filename using the default target) we can just ignore the label in the code and use the filename here:

https://github.com/semantic-release/gitlab/blob/master/lib/publish.js#L88

IMO is better to give the ability to users to use their own label. BTW This is not a blocking behavior, when the target is to generic_package using the filename as label create the release correctly.

This is a little bit more tricky then 1..

yep man, it is lol 🤣

For me it looks like you use Google Storage to store some assets and packages and this provider returns an URL with an expiration date. Not sure whether this is for the URL only or also for the file. Interesting would be what happens if we use the uploadEndpoint? Even if we use this URL isn't the returned URL still active in the background and by this probably causing the same issue? Would it be possible for you to test this in your project?

yes, I already tested it and it works :) I have a fork of the project with this fix and I can confirm resolve the issue.

BTW the currently used URL is the one provided by the API call response according to the docs. With this said, the docs also highlight how to download a generic package.

Yes you are right, but I guess is not well documented, I'll try to find the code on gitlab repo to check better the behavior. What can I say is that my builds are greater the 200mb, probably gitlab uses google storage if the size of the package is bigger than a certain value. I don't know. But the url provided in filename is broken in my case, and this is a blocking bug that doesn't allow me to use this kind of release

But:

If multiple packages have the same name, version, and filename, then the most recent one is retrieved.

With the current approach we have direct access to the uploaded file without worrying about this drawback, which we might loose using the Gitlab URL.

To be honest I don't understand which is this case, let's assume I'll build two different packages with the same name, for example:

  • /build/x86/MyApp.dmg <-for x86
  • /build/arm64/MyApp.dmg <-for arm64

And try to publish them. In this case I will have an url like:
/projects/${encodedRepoId}/packages/generic/release/my-tag-v1.0/MyApp.dmg

That of course will store the last published file to that path. At the end you will always get the most recent (last published) package downloading the content from the package path

The main problem is that it doesn't works for me as is now, I have to use my fork.

@mfoti
Copy link

mfoti commented Feb 15, 2023

I guess there is another feature that can be used too, the package type.

At the moment it is hardcoded:

assetsList.push({ label, alt: "release", url, type: "package", filepath, target }); (https://github.com/semantic-release/gitlab/blob/master/lib/publish.js#L107)

This type value should be taken from the configuration, this should fix:

const type = asset.type ? template(asset.type)(context) : "package";
assetsList.push({ label, alt: "release", url, type, filepath, target });

The final effect, without hardcoding the type, will be this:
Screen Shot 2023-02-15 at 12 04 03

@JonasSchubert
Copy link
Contributor Author

JonasSchubert commented Feb 16, 2023

@mfoti You already created a fork with a fix on your side. Feel free to create the PR ;)
Already thanks for your work!

@mfoti
Copy link

mfoti commented Feb 16, 2023

I should squash, but is a really easy fix: #504, this fixes only the main "bug", but not the package type and the filename/label

@JonasSchubert
Copy link
Contributor Author

I guess there is another feature that can be used too, the package type.

At the moment it is hardcoded:

assetsList.push({ label, alt: "release", url, type: "package", filepath, target }); (https://github.com/semantic-release/gitlab/blob/master/lib/publish.js#L107)

IMO the current implementation is correct here. Generic packages only should have the type packages. The other type options (runbook, image and other) are for assets and not packages.
This can be used with either not setting the property target at all or setting the (default) value project_upload.

@JonasSchubert
Copy link
Contributor Author

I guess defaults to the filename, but allow to put a string as package description, as in my case: "MacOS x64 (.dmg)"

BTW I can confirm that using the filename as label works:

       // This works
       {
          label: `MyApp-${nextRelease.version}-x64.dmg`,
          path: `release/build/MyApp-${nextRelease.version}-x64.dmg`,
          type: "image",
          target: "generic_package"
        },

But if you use a different label it doesn't

       // This doesn't works
       {
          label: 'MacOS x64 (.dmg)',
          path: `release/build/MyApp-${nextRelease.version}-x64.dmg`,
          type: "image",
          target: "generic_package"
        },

I guess to avoid errors (release is correctly generated with a label different of the filename using the default target) we can just ignore the label in the code and use the filename here:

https://github.com/semantic-release/gitlab/blob/master/lib/publish.js#L88

IMO is better to give the ability to users to use their own label. BTW This is not a blocking behavior, when the target is to generic_package using the filename as label create the release correctly.

I think this is a create idea. Would you like to add it to your PR too?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants