-
Notifications
You must be signed in to change notification settings - Fork 79
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
fix: use channel as package name for generic_package upload #607
base: master
Are you sure you want to change the base?
fix: use channel as package name for generic_package upload #607
Conversation
It actually does not correct if the configuration provide a regex path (may match multiple files to upload). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO the option to configure the package name is a good idea.
But I would only provide this configuration option and would not change other functionalities.
Unit tests should be added, too.
@@ -88,11 +88,14 @@ export default async (pluginConfig, context) => { | |||
|
|||
if (target === "generic_package") { | |||
// Upload generic packages | |||
const encodedLabel = encodeURIComponent(label); | |||
const { major, minor, patch } = semver.parse(version); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand that correct, this renders it impossible to use prereleases (alpha, beta).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this does not look correct. GitLab allows much more:
The package version. The following regex validates this: \A(.?[\w+-]+.?)+\z. You can test your version strings on Rubular.
// https://docs.gitlab.com/ee/user/packages/generic_packages/#publish-a-package-file | ||
uploadEndpoint = urlJoin( | ||
gitlabApiUrl, | ||
`/projects/${encodedRepoId}/packages/generic/release/${encodedVersion}/${encodedLabel}?${ | ||
`/projects/${encodedRepoId}/packages/generic/${encodedChannel}/${encodedVersion}/${encodedLabel}?${ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the Gitlab documentation this changed part is the package name.
I would suggest, if we change this, to either use the package name or allow to provide a name for assets.
IMO the semantic release channel is not the correct way to go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does look clean and well organized, yes.
My concerns are regarding users trying to use package names instead of channels.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see.
It's better add it as an optional configuration.
Old users will not be forced to change their existing projects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the structure too, but I would not enforce it to all users.
Maybe let's just a configuration option for asset.packageName
and make that templatable. Then you could configure it something like this:
{
"branches": ["main"],
"plugins": [
[
"@semantic-release/gitlab",
{
"assets": [
{
"path": "dist/asset.min.js",
"label": "JS distribution",
"target": "generic_package",
"packageName": "${nextRelease.channel}"
}
]
}
]
]
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fgreinacher thank you for the suggestion.
I'm implementing it that way and will push new code soon.
const encodedLabel = encodeURIComponent(label); | ||
const { major, minor, patch } = semver.parse(version); | ||
const encodedVersion = `${major}.${minor}.${patch}`; | ||
const encodedLabel = encodeURIComponent(label ?? version); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO the fallback to the version is confusing. The version will then be the file name and package version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I notice this too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
asset.label
can be templated, so that should be solvable via configuration, see https://github.com/semantic-release/gitlab#usage, and we don't need to change the core logic.
I would update the implementation and add unit tests later when the implementation has been approved. |
@phuocnb Feel free to go on and ping me if you're stuck somewhere! |
I were so busy in the last 2 months. |
@fgreinacher @JonasSchubert
The upload request actually return a This is the full job log After running the above job, the result is:
Changing the
In the line 98-103
line 115-118
I believe that This is the original issue that i want to commit a fix on this PR. Anyway, i still working on this issue (on another branch). |
The current
generic_package
upload does not satisfy what actual needs. We don't always release on the officialrelease
channel.According to gitlab url format:
/projects/:id/packages/generic/:package_name/:package_version/:file_name?status=:status
What i did:
package_name
equal tochannel
(release, alpha, beta, rc, etc)package_version
always requires in formatmajor.minor.patch
(required from gitlab generic package upload)file_name
is treated as the label config (if the label value does not exist, it would takeversion
value instead)