Skip to content

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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions lib/glob-assets.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
const path = require('path');
const {basename} = require('path');
const {isPlainObject, castArray, uniqWith, uniq} = require('lodash');
const dirGlob = require('dir-glob');
const globby = require('globby');
const debug = require('debug')('semantic-release:gitlab');

module.exports = async ({cwd}, assets) =>
uniqWith(
module.exports = async ({pkgRoot, cwd}, assets) => {
return uniqWith(
Comment on lines +7 to +8
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

[]
.concat(
...(await Promise.all(
@@ -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.

}

// If asset is an Object, output an Object definition with:
@@ -66,3 +65,4 @@ module.exports = async ({cwd}, assets) =>
// Compare `path` property if Object definition, value itself if String
(a, b) => path.resolve(cwd, isPlainObject(a) ? a.path : a) === path.resolve(cwd, isPlainObject(b) ? b.path : b)
);
};
2 changes: 1 addition & 1 deletion lib/publish.js
Original file line number Diff line number Diff line change
@@ -30,7 +30,7 @@ module.exports = async (pluginConfig, context) => {
debug('milestones: %o', milestones);

if (assets && assets.length > 0) {
const globbedAssets = await getAssets(context, assets);
const globbedAssets = await getAssets({cwd: context.cwd, pkgRoot: pluginConfig.pkgRoot}, assets);
debug('globbed assets: %o', globbedAssets);

await Promise.all(
33 changes: 16 additions & 17 deletions lib/resolve-config.js
Original file line number Diff line number Diff line change
@@ -1,23 +1,21 @@
const {castArray, isNil} = require('lodash');
const urlJoin = require('url-join');

module.exports = (
{gitlabUrl, gitlabApiPathPrefix, assets, milestones},
{
envCi: {service} = {},
env: {
CI_PROJECT_URL,
CI_PROJECT_PATH,
CI_API_V4_URL,
GL_TOKEN,
GITLAB_TOKEN,
GL_URL,
GITLAB_URL,
GL_PREFIX,
GITLAB_PREFIX,
},
}
) => {
module.exports = (pluginConfig, context) => {
const {gitlabUrl, gitlabApiPathPrefix, assets, milestones, pkgRoot} = pluginConfig;
const {service} = context.envCi || {service: undefined};
const {
CI_PROJECT_URL,
CI_PROJECT_PATH,
CI_API_V4_URL,
GL_TOKEN,
GITLAB_TOKEN,
GL_URL,
GITLAB_URL,
GL_PREFIX,
GITLAB_PREFIX,
} = context.env;

const userGitlabApiPathPrefix = isNil(gitlabApiPathPrefix)
? isNil(GL_PREFIX)
? GITLAB_PREFIX
@@ -31,6 +29,7 @@ module.exports = (
: 'https://gitlab.com');

return {
pkgRoot,
Copy link
Contributor

@fgreinacher fgreinacher Oct 10, 2023

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

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 🙇

gitlabToken: GL_TOKEN || GITLAB_TOKEN,
gitlabUrl: defaultedGitlabUrl,
gitlabApiUrl:
11 changes: 11 additions & 0 deletions test/resolve-config.test.js
Original file line number Diff line number Diff line change
@@ -14,6 +14,7 @@ test('Returns user config', t => {
gitlabApiUrl: urlJoin(gitlabUrl, gitlabApiPathPrefix),
assets,
milestones: undefined,
pkgRoot: undefined,
});
});

@@ -35,6 +36,7 @@ test('Returns user config via environment variables', t => {
gitlabApiUrl: urlJoin(gitlabUrl, gitlabApiPathPrefix),
assets,
milestones,
pkgRoot: undefined,
}
);
});
@@ -53,6 +55,7 @@ test('Returns user config via alternative environment variables', t => {
gitlabApiUrl: urlJoin(gitlabUrl, gitlabApiPathPrefix),
assets,
milestones: undefined,
pkgRoot: undefined,
}
);
});
@@ -68,6 +71,7 @@ test('Returns default config', t => {
gitlabApiUrl: urlJoin('https://gitlab.com', '/api/v4'),
assets: undefined,
milestones: undefined,
pkgRoot: undefined,
});

t.deepEqual(resolveConfig({gitlabApiPathPrefix}, {env: {GL_TOKEN: gitlabToken}}), {
@@ -76,6 +80,7 @@ test('Returns default config', t => {
gitlabApiUrl: urlJoin('https://gitlab.com', gitlabApiPathPrefix),
assets: undefined,
milestones: undefined,
pkgRoot: undefined,
});

t.deepEqual(resolveConfig({gitlabUrl}, {env: {GL_TOKEN: gitlabToken}}), {
@@ -84,6 +89,7 @@ test('Returns default config', t => {
gitlabApiUrl: urlJoin(gitlabUrl, '/api/v4'),
assets: undefined,
milestones: undefined,
pkgRoot: undefined,
});
});

@@ -107,6 +113,7 @@ test('Returns default config via GitLab CI/CD environment variables', t => {
gitlabApiUrl: CI_API_V4_URL,
assets: undefined,
milestones: undefined,
pkgRoot: undefined,
}
);
});
@@ -134,6 +141,7 @@ test('Returns user config over GitLab CI/CD environment variables', t => {
gitlabApiUrl: urlJoin(gitlabUrl, gitlabApiPathPrefix),
assets,
milestones: undefined,
pkgRoot: undefined,
}
);
});
@@ -167,6 +175,7 @@ test('Returns user config via environment variables over GitLab CI/CD environmen
gitlabApiUrl: urlJoin(gitlabUrl, gitlabApiPathPrefix),
assets: undefined,
milestones: undefined,
pkgRoot: undefined,
}
);
});
@@ -200,6 +209,7 @@ test('Returns user config via alternative environment variables over GitLab CI/C
gitlabApiUrl: urlJoin(gitlabUrl, gitlabApiPathPrefix),
assets: undefined,
milestones: undefined,
pkgRoot: undefined,
}
);
});
@@ -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.