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

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
Show file tree
Hide file tree
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(
Expand Down Expand Up @@ -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:
Expand All @@ -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
Expand Up @@ -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(
Expand Down
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
Expand All @@ -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:
Expand Down
11 changes: 11 additions & 0 deletions test/resolve-config.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ test('Returns user config', t => {
gitlabApiUrl: urlJoin(gitlabUrl, gitlabApiPathPrefix),
assets,
milestones: undefined,
pkgRoot: undefined,
});
});

Expand All @@ -35,6 +36,7 @@ test('Returns user config via environment variables', t => {
gitlabApiUrl: urlJoin(gitlabUrl, gitlabApiPathPrefix),
assets,
milestones,
pkgRoot: undefined,
}
);
});
Expand All @@ -53,6 +55,7 @@ test('Returns user config via alternative environment variables', t => {
gitlabApiUrl: urlJoin(gitlabUrl, gitlabApiPathPrefix),
assets,
milestones: undefined,
pkgRoot: undefined,
}
);
});
Expand All @@ -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}}), {
Expand All @@ -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}}), {
Expand All @@ -84,6 +89,7 @@ test('Returns default config', t => {
gitlabApiUrl: urlJoin(gitlabUrl, '/api/v4'),
assets: undefined,
milestones: undefined,
pkgRoot: undefined,
});
});

Expand All @@ -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,
}
);
});
Expand Down Expand Up @@ -134,6 +141,7 @@ test('Returns user config over GitLab CI/CD environment variables', t => {
gitlabApiUrl: urlJoin(gitlabUrl, gitlabApiPathPrefix),
assets,
milestones: undefined,
pkgRoot: undefined,
}
);
});
Expand Down Expand Up @@ -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,
}
);
});
Expand Down Expand Up @@ -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,
}
);
});
Expand All @@ -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.