Skip to content

fix(publish): Compatible with older versions of GitLab, lack of "name" attribu… #240

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

Closed
wants to merge 4 commits into from

Conversation

ldc2726
Copy link

@ldc2726 ldc2726 commented May 26, 2021

Compatible with older versions of GitLab, lack of "name" attribute, request result return 400 codete, request result return 400 code

@ldc2726 ldc2726 mentioned this pull request May 26, 2021
Copy link
Contributor

@nfriend nfriend left a comment

Choose a reason for hiding this comment

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

Thanks for this @ldc2726, looking good!

Two small comments in publish.js. It also looks like there are a few PR checks failing - can you take a look?

Also, let's make sure this new logic has test coverage in both publish.test.js and integration.test.js.

Back to you for now!

lib/publish.js Outdated
@@ -72,6 +73,7 @@ module.exports = async (pluginConfig, context) => {
...apiOptions,
json: {
/* eslint-disable camelcase */
name:name ? name : gitTag,
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for this - this matches GitLab's default behavior in version 12+.

nitpick: I personally like this phrasing better:

Suggested change
name:name ? name : gitTag,
name: name || gitTag

lib/publish.js Outdated
@@ -25,7 +25,8 @@ module.exports = async (pluginConfig, context) => {
const apiOptions = {headers: {'PRIVATE-TOKEN': gitlabToken}};

debug('repoId: %o', repoId);
debug('release name: %o', gitTag);
debug('release name: %o', name)
Copy link
Contributor

Choose a reason for hiding this comment

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

small issue: if name is falsy, this will output something like:

release name: undefined

However, the actual network request will be made with name: gitTag.

Let's make this log message consistent with what's actually being sent by computing the name above this line and then using the computed name here and in the actual network request:

const releaseName = name || gitTag;

// ...

debug('release name: %o', releaseName)

// ...

json: {
  name: releaseName
}

@nfriend
Copy link
Contributor

nfriend commented Jun 4, 2021

@ldc2726 Is this ready for another review? Looks like the checks are still failing so I'm not sure if I should take another look. Feel free to ping me when this is ready 😄

@alecgerona
Copy link

Maybe we can also log the failure reason?

@nfriend
Copy link
Contributor

nfriend commented Jun 17, 2021

Maybe we can also log the failure reason?

@alecgerona I opened a PR to do some better logging in #245 👍

@nfriend
Copy link
Contributor

nfriend commented Jun 17, 2021

@ldc2726 Let me know if you need any help getting the merge checks green.

@nfriend
Copy link
Contributor

nfriend commented Aug 5, 2021

@ldc2726 Sorry for the slow turnaround time on this PR - I've been out for the last several weeks.

After some discussion about this change (see #246), we've decided not to merge this fix since it's only applicable to an officially unsupported version of GitLab (v11). This project's README has since been updated to clarify our compatibility guidelines:

The latest version of this plugin is compatible with all currently-supported versions of GitLab, which is the current major version and previous two major versions. This plugin is not guaranteed to work with unsupported versions of GitLab.

This guideline allows us to minimize the maintenance burden of this plugin, since all of the maintainers of this plugin are volunteers.

Sorry for the inconvenience this is causing/will cause you! I appreciate your participation in this project.

@nfriend nfriend closed this Aug 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants