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.

Sign up for GitHub

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

Jump to bottom

fix: file labels in GitLab releases, to ensure they're unique. #267

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix: file labels in GitLab releases, to ensure they're unique. #267

wants to merge 1 commit into from

Conversation

EvanCarroll
Copy link

@EvanCarroll EvanCarroll commented Jul 29, 2021 •
edited
Loading

Previously GitLab lables were just the basename for files uploaded as
part of the release. This is problematic because GitLab doesn't allow
conflicting labels -- a condition that could be caused by uploading a
release with two files by the same name in different directories. This
would generate a 409 Conflict error.

This changes the labels for files uploaded as part of a release to the
name relative to pkgRoot, or the package.

A project may look like this

pkg
pkg \ foo \ baz
pkg \ bar \ baz

This would previously result in two conflicting labels of 'baz'. Now you
would have {"foo/baz", "bar/baz"} with no conflict.

GitHub issues: #265, #158

@EvanCarroll EvanCarroll force-pushed the pkgroot branch 2 times, most recently from b33ed93 to 9be4ada Compare July 29, 2021 19:08
@EvanCarroll
Copy link
Author

The force pushes are all commit message. I linked it to the issues and fixed some typos.

@EvanCarroll
Copy link
Author

It may be worth documenting too, but you would use this feature like this

    ["@semantic-release/gitlab", {
      "gitlabUrl": "https://gitlab.awe.eco.cpanel.net/",
      "pkgRoot": "dist/cpanel/angular-validators",
      "assets": [
        {"path": "dist/cpanel/angular-validators/**", "label": "Dist output"}
      ]   
    }]

Now all things found in dist/cpanel/angular-validators/ have their label in the release relative to the pkgRoot

@EvanCarroll
fix: file labels in GitLab releases, to ensure they're unique.
04f20f2 
Previously GitLab lables were just the basename for files uploaded as
part of the release. This is problematic because GitLab doesn't allow
conflicting labels -- a condition that could be caused by uploading a
release with two files by the same name in different directories. This
would generate a 409 Conflict error.

This changes the labels for files uploaded as part of a release to the
name relative to pkgRoot, or the package.

A project may look like this

  pkg
  pkg \ foo \ baz
  pkg \ bar \ baz

This would previously result in two conflicting labels of 'baz'. Now you
would have {"foo/baz", "bar/baz"} with no conflict.

GitHub issues: semantic-release#265, semantic-release#158
@EvanCarroll
Copy link
Author

I rebased this can I get some input on it?

@dimooze dimooze mentioned this pull request Jan 11, 2022
Open
@fgreinacher
Copy link
Contributor

This PR has been stale for nearly two years now, so I'm closing it.

Feel free to send a new PR if you want this 🙇

@fgreinacher fgreinacher closed this Oct 5, 2023
@EvanCarroll
Copy link
Author

EvanCarroll commented Oct 5, 2023 •
edited
Loading

It was stale because you never gave any input on it lol. Despite being asked by others in the same issues #265 (comment)

@fgreinacher
Copy link
Contributor

fgreinacher commented Oct 5, 2023 •
edited
Loading

Ha wow, too ambitious in cleaning up. Sorry!

@EvanCarroll I'll review tomorrow or early next week. Can you rebase once more? 🙇

@fgreinacher fgreinacher reopened this Oct 5, 2023
@fgreinacher fgreinacher self-requested a review October 5, 2023 16:06
fgreinacher
fgreinacher reviewed Oct 10, 2023
View reviewed changes
lib/glob-assets.js
@@ -42,7 +41,7 @@ module.exports = async ({cwd}, assets) =>
// - `filepath` ignored (also to avoid duplicates)
// - other properties of the original asset definition
const {filepath, ...others} = asset;
return globbed.map(file => ({...others, path: file, label: basename(file)}));
return globbed.map(file => ({...others, path: file, label: path.relative(pkgRoot || '.', file)}));
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we consider this a breaking change?

Copy link
Author

Choose a reason for hiding this comment

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

@fgreinacher I wouldn't unless generating duplicate labels was support somewhere and things depended o nit. Honestly don't know. Seems weird. Imagine creating a listing of files in multiple directories and losing the directory they were in, that was the old behavior. So the labels would collide and the SCM would reject the operation.

fgreinacher
fgreinacher reviewed Oct 10, 2023
View reviewed changes
lib/resolve-config.js
@@ -31,6 +29,7 @@ module.exports = (
: 'https://gitlab.com');

return {
pkgRoot,
Copy link
Contributor

@fgreinacher fgreinacher Oct 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I understand you are mirroring the NPM plugin here, but the concept of a package does not really exist for the GitLab plugin.

What about assetBasePath or assetRootPath?

Copy link
Contributor

@fgreinacher fgreinacher Oct 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Also please add some documentation 🙇

fgreinacher
fgreinacher reviewed Oct 10, 2023
View reviewed changes
test/resolve-config.test.js
@@ -224,6 +234,7 @@ test('Ignore GitLab CI/CD environment variables if not running on GitLab CI/CD',
gitlabApiUrl: urlJoin('https://gitlab.com', '/api/v4'),
assets: undefined,
milestones: undefined,
pkgRoot: undefined,
}
);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add some test cases for the new option? Especially weird cases like when the asset is not within the base/root path or when the base/root path is somehow malformed/does not exist.

fgreinacher
fgreinacher reviewed Oct 10, 2023
View reviewed changes
lib/glob-assets.js
Comment on lines +7 to +8
module.exports = async ({pkgRoot, cwd}, assets) => {
return uniqWith(
Copy link
Contributor

Choose a reason for hiding this comment

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

This can stay an arrow function, no?

Copy link
Author

Choose a reason for hiding this comment

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

@fgreinacher I'm confused. It's still an arrow function

fgreinacher
fgreinacher requested changes Oct 10, 2023
View reviewed changes
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.

Thanks @EvanCarroll and sorry that I somewhat missed that this is ready for review.

I left some comments :)

@fgreinacher
Copy link
Contributor

@EvanCarroll Do you still want to tackle this? If not someone else could take over.

@JonasSchubert
Copy link
Contributor

@EvanCarroll shall somebody else take over for you or do you want to continue here?

@fgreinacher
Copy link
Contributor

I think somebody else needs to take over here.

@EvanCarroll
Copy link
Author

Please, take it over. I'm not adding tests for this. If you want to add tests to a project that didn't previously have them, that's another issue imho.

Besides I have to maintain a forks of all of the bugs I've fixed in the semantic-release projects anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fgreinacher fgreinacher fgreinacher requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@EvanCarroll @fgreinacher @JonasSchubert