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

wrong Gitlab issue link in release notes #449

Open
safwatsafi opened this issue May 18, 2023 · 20 comments
Open

wrong Gitlab issue link in release notes #449

safwatsafi opened this issue May 18, 2023 · 20 comments

Comments

@safwatsafi
Copy link

safwatsafi commented May 18, 2023

Hello,

in Release notes the links of issues and commits are not correct. the link is missing "-/"

for example:
the link in release notes:
Repository_URL/issues/<issue_iid>
the link in Gitlab:
Repository_URL/-/issues/<issue_iid>

any idea how to fix this issue?

@doteric
Copy link

doteric commented May 18, 2023

I have the exact same issue for almost every url. Every url is missing /-/.
For example:

  • currently https://gitlab.com/repo_name/compare/1.5.1...1.5.2 , should be https://gitlab.com/repo_name/-/compare/1.5.1...1.5.2
  • currently https://gitlab.com/repo_name/commit/xxxx, should be https://gitlab.com/repo_name/-/commit/xxxx

I bumped the version of all packages expecting that this was already fixed, but it looks like the issue is still present on the newest version. It looks like GitLab no longer supports urls without the /-/.

CC @travi

@travi
Copy link
Member

travi commented May 19, 2023

from my understanding, the underlying conventional-changelog-writer should handle details like this automatically as long as your project is configured properly to specify that your project is on gitlab. for example, this sounds related to conventional-changelog/conventional-changelog#257

@fgreinacher are you familiar with the details necessary to get this working as expected?

@travi
Copy link
Member

travi commented May 19, 2023

if you execute with the --debug flag, we do output the details that are passed to conventional-changelog-writer. could you share what that shows being detected for repository?

@fgreinacher
Copy link

I have never noticed that. Might be a change in GitLab 16?

@sylvainOL
Copy link

I guess this is what made the change:: https://docs.gitlab.com/ee/update/deprecations.html?removal_milestone=16.0#legacy-urls-replaced-or-removed

@travi
Copy link
Member

travi commented May 20, 2023

This sounds like something that would be better handled in conventional-changelog-writer than specifically in semantic-release. Could one of you start a conversation there and reference it from here?

@safwatsafi
Copy link
Author

safwatsafi commented May 21, 2023

we tried this for commits and issues:
"[semantic-release] › ℹ Start step "generateNotes" of plugin "@semantic-release/release-notes-generator"
semantic-release:release-notes-generator version: '1.1.0' +0ms
semantic-release:release-notes-generator host: undefined +0ms
semantic-release:release-notes-generator owner: 'xxxxxxxx' +0ms
semantic-release:release-notes-generator repository: 'xxxxxxxx/semantic-test' +0ms
semantic-release:release-notes-generator previousTag: 'v1.0.0' +0ms
semantic-release:release-notes-generator currentTag: 'v1.1.0' +1ms
semantic-release:release-notes-generator host: 'https://gitlab.com' +0ms
semantic-release:release-notes-generator linkReferences: true +0ms
semantic-release:release-notes-generator issue: '-/issues' +0ms
semantic-release:release-notes-generator commit: '-/commit' +0ms
[3:24:25 PM] [semantic-release] › ✔ Completed step "generateNotes" of plugin "@semantic-release/release-notes-generator"
[3:24:25 PM] [semantic-release] › ⚠ Skip v1.1.0 tag creation in dry-run mode
[3:24:25 PM] [semantic-release] › ✔ Published release 1.1.0 on default channel
[3:24:25 PM] [semantic-release] › ℹ Release note for version 1.1.0:

1.1.0 (https://gitlab.com/xxxxxxxx/semantic-test/compare/v1.0.0...v1.1.0) (2023-05-18)

Features

* #2 (https://gitlab.com/xxxxxxxx/semantic-test/issues/2) test issue 2 (10793e9 (https://gitlab.com/xxxxxxxx/semantic-test/-/commit/10793e987dxx3f3dee42be6b4e95cf4be45660ef))"

As you see, it worked for commits (/-/commit/) but not for issues

@doteric
Copy link

doteric commented May 21, 2023

Hey @travi ,
As I've already mentioned here , there is no already available way for the write to handle this. The easiest approach would be to append the repository url, like I've prepared in the PR, otherwise a lot of things would need to be reworked as it's not only commits and issues that are the problem, but almost every GitLab url.

@travi
Copy link
Member

travi commented May 21, 2023

there is no already available way for the write to handle this

help me understand. conventional-changlelog-writer currently has logic to vary it's output based on the repository host, which has been working for gitlab up to this point from my understanding. if gitlab has changed their format and conventional-changelog-writer is producing incorrect output for gitlab because it still uses the older format, it seems like the best approach would be for the logic within conventional-changelog-writer to be updated to cover the gitlab changes. since conventional-changelog-writer has more consumers than just semantic-release, it would be better for the update to improve the situation for consumers beyond semantic-release

are you trying to highlight something that is beyond the understanding that i'm outlining here?

@doteric
Copy link

doteric commented May 22, 2023

@travi
So the problem is that the conventional-changelog-writer does not have any logic to support different urls based on what host is provided, this would be awful especially if the host was some custom one (self-hosted instances for example). It just accept certain parameters and uses them mostly in a clean way. The logic based on hosts is actually inside the release-notes-generator here. Inside there you modify specific values based on what git host is used, therefore it seems like release-notes-generator is the correct place to do this 🤔 Correct me if I'm wrong though as I just quickly went through the files to check the logic more or less inside both repositories.

@doteric
Copy link

doteric commented May 24, 2023

So @travi what do you think about the PR?
#450
Do we want to do it that way or some different way? Let's decide on something as it's a little bit annoying that all the GitLab urls are wrong.

@travi
Copy link
Member

travi commented May 24, 2023

@doteric please stop pinging maintainers directly for updates.

i understand that this feels urgent from your perspective, but we have to ask for your patience with this. i would never tell someone to delay updating to the latest version of a major tool, but by upgrading to a version that includes changes that break tools like semantic-release, that is a consequence of making that upgrade. before our users, like you, started reporting the breakage to us, we were unaware that this breakage was coming.

what do you think about the PR?

as i mentioned above, i believe the better place to fix this is in conventional-changelog. if you do a search in that project for "gitlab" you can find several previous conversations about formatting of gitlab urls. i could be wrong about that being the proper place, but one of the reasons we ask for patience is to have a chance to investigate further and hear back from the maintainers of conventional-changelog. in addition, semantic-release is not a full time project for any of us, so time available for our investigation is limited. beyond that context, we try very hard to avoid configuration that enables arbitrary content to be provided by users that is then injected into output like a link. it is very unlikely that we would accept such an approach unless there is no better option (and would likely require some level of input sanitization).

from my understanding, this gitlab update also broke links contained in previous releases. a fix in semantic-release would only impact links generated for new releases. until a resolution is found, new links will be broken in the same way as your previous releases. it is up to you if you go back and update previous releases, but such changes could be applied the same way to the releases impacted by this time period. another workaround could be to disable linkReferences in the config for this plugin.

@doteric
Copy link

doteric commented May 24, 2023

Thanks for the reply Travi,

please stop pinging maintainers directly for updates.
i understand that this feels urgent from your perspective, but we have to ask for your patience with this. i would never tell someone to delay updating to the latest version of a major tool, but...

Sorry for bothering, don't want to cause problems and just a reply whenever you have some spare time is perfectly fine. Anyway there is nothing one could do, it's a breaking change on GitLab's side and in such case we can't just go to a previous version therefore it simply does not work anymore and will not until addressed, but I understand your concern also.

as i mentioned above, i believe the better place to fix this is in conventional-changelog...

I fully understand your concerns, but the thing is that conventional-changelog does not seem to have any logic inside the writer to distinguish between different types of git hosts. It simply accepts a repository url and does not care what it actually is, it's up to the consumer (release-notes-generator in this case) how it should actually look. The only reasonable change that conventional-changelog could do is add something like a repositorySuffix as an accepted parameter and simply do the same thing I'm doing in the PR, but inside their package. The other thing that could be done is the -/ could be added to the commit (and every other path parameter) to look like so -/commits (for example), but that would require adding extra parameters for compare for example. On the other hand the lib/hosts-config.js seems like the thing we want to do (separate configuration between hosts) and it's available within the release-notes-generator, that's why my reasoning to put it inside here as we are already doing this for other things (for example changing issue path, commit path, issues prefix etc. all done inside the hosts-config.js, this is also an url modification). What we could add to the PR is to allow only certain characters in the suffix so that someone doesn't escape the url into some completely different domain, but at the same time someone could do it either way with other options).

from my understanding, this gitlab update also broke links contained in previous releases. a fix in semantic-release would only impact links generated for new releases...

Yes exactly, a fix would not fix previous releases anyway, but at least new releases would not be impacted.

So to conclude - you are right that there is not big hurry and let's wait and see if there is a better solution on a different level. 👍
For now we will just manually be updating the links if we require them to be correct (for example for reports).

Sorry for bothering again and wish you a good day 🙇

@travi
Copy link
Member

travi commented May 25, 2023

The logic based on hosts is actually inside the release-notes-generator here. Inside there you modify specific values based on what git host is used, therefore it seems like release-notes-generator is the correct place to do this 🤔 Correct me if I'm wrong though as I just quickly went through the files to check the logic more or less inside both repositories.

as i continue to investigate this, i think i'm finally grasping what you were clarifying here. these details were defined this way before i joined the project as a maintainer, so i was making an assumption that appears to be incorrect. i thought we were leveraging the host details defined in the core package from the conventional-changelog project.

since the configs appear to be the same at first glance between the projects, it left me wondering why we have our own definition. it appears that our version was defined a few months ahead of the version exposed in the other package, or at least ahead of when it was exposed in the current way. i havent compared the two sets of configs in detail, but assuming further inspection doesnt expose a reason to keep them separate, that duplication feels like debt that should be cleaned up rather than making a change that causes them to diverge.

that sort of cleanup obviously would not solve the gitlab breaking change situation, but i think it does get us in line with my earlier understanding where the conventional-changelog project would be the proper source of those rules. aligning on a way to make the update there would then also enable fixing use with semantic-release and with other consumers of those rules through various uses of the conventional-changelog-core package.

assuming no surprises would arise from taking such a step in this plugin, are there concerns that come up that i might not be considering?

@travi
Copy link
Member

travi commented May 25, 2023

we try very hard to avoid configuration that enables arbitrary content to be provided by users that is then injected into output like a link. it is very unlikely that we would accept such an approach unless there is no better option (and would likely require some level of input sanitization).

given my misunderstanding of the hosts config, i was not fully processing the proposal to define the extra path part as part of the hosts config. instead, i was understanding the proposal as open user input as part of the plugin config. i now understand that this would be less freeform than i thought.

however, this does raise another question that i thought would have been handled by opting into the extra path part through plugin config. how is semantic-release or conventional-changelog to know which version of gitlab is in use? is my understanding correct that there are supported versions of gitlab that still use the old format? wouldn't using the new format for all github projects just break those that have not upgraded instead of those that have upgraded?

@sylvainOL
Copy link

for info, the "new" urls were deployed in gitlab 9.0 5 years ago so I'm pretty sure everybody has jumped to the new pattern.

@doteric
Copy link

doteric commented May 25, 2023

given my misunderstanding of the hosts config, i was not fully processing the proposal to define the extra path part as part of the hosts config. instead, i was understanding the proposal as open user input as part of the plugin config. i now understand that this would be less freeform than i thought.

No worries, you are kinda right on one part though, these values can also be edited by the plugin payload/config and in case someone has a self-hosted instance then all of them will need to be inputed manually anyway (at least from what I understand from the code).

Edit: Not all values can actually be edited, just the ones provided here: https://github.com/semantic-release/release-notes-generator/blob/master/index.js#LL77C43
So we can add the repositoryPathSuffix parameter to being editable also if we'd like.

however, this does raise another question that i thought would have been handled by opting into the extra path part through plugin config. how is semantic-release or conventional-changelog to know which version of gitlab is in use? is my understanding correct that there are supported versions of gitlab that still use the old format? wouldn't using the new format for all github projects just break those that have not upgraded instead of those that have upgraded?

According to https://github.com/semantic-release/release-notes-generator/blob/master/index.js#L45 (and https://github.com/semantic-release/release-notes-generator/blob/master/lib/hosts-config.js#LL30C5-L30C13) the hostname needs to match the one provided in the config that means that if someone is using a self-hosted GitLab instance then it won't match anyway and there is no way to use an old version of the gitlab.com SaaS therefore there is no chance somebody is still using an old version anyway ;D

@baby-gnu
Copy link

I override the configuration as described in the conventional-changelog documentation

const config = {
  plugins: [
    [
      '@semantic-release/release-notes-generator',
      {
        presetConfig:
        {
          commitUrlFormat: '{{host}}/{{owner}}/{{repository}}/-/commit/{{hash}}',
          compareUrlFormat: '{{host}}/{{owner}}/{{repository}}/-/compare/{{previousTag}}...{{currentTag}}',
          issueUrlFormat: '{{host}}/{{owner}}/{{repository}}/-/issues/{{id}}',
        },
      },
    ],
  ],
};

@travi
Copy link
Member

travi commented Jun 2, 2023

i've dug further into the refactor that i mentioned above to attempt to leverage the hosts config from the core conventional-changelog package. while i do think there is direct duplication of the current hosts config between our two projects, i'm not seeing a clean way to share that config from conventional-changelog and also handle use of that config in a way that is compatible with our context. in addition, while that code would be accessible from the conventional-changelog package, it isnt defined to be part of their public api, so we would be taking a risk of being broken from that being made inaccessible in the future.

given that context, it does seem like it would be on us if we want to handle this correctly for this plugin when gitlab is used. the one hesitation that i still with the proposed change in #450 is considering the generic approach to be simply a suffix to the repository definition. that does work in this scenario, but because the change that gitlab decided to make happens to be between the repository and the rest of the path parts. plus, they applied it consistently across all of the links necessary.

@baby-gnu thank you for sharing the approach that you found to work. the thing i like about that approach is that it handles the various links independently and would enable changes at other positions in the url. i have not had a chance to determine if this sort of approach is possible directly through the writer api, but i think i would lean in this sort of direction if it is possible. if someone would be willing to determine if something like this would be possible in our call to the writer, that would be helpful for moving this forward.

@doteric
Copy link

doteric commented Jun 19, 2023

Just FYI.
It looks like GitLab decided to reintroduce the redirect some time ago, therefore this issue can be brought back to low priority.

As for the format urls I'm not really a fan of having to set each one of those (not even sure if it would work in all cases anyway) especially that we know that every GitLab url changes. Up to you to decide though ;D

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

No branches or pull requests

6 participants