fix(assets): changed generation of gitlab release asset url #754
Conversation
lib/publish.js
Outdated
| @@ -149,7 +149,8 @@ export default async (pluginConfig, context) => { | |||
| throw error; | |||
| } | |||
|
|
|||
| const { url, alt } = response; | |||
| const { alt } = response; | |||
| const url = urlJoin(gitlabUrl, response["full_path"]); | |||
There was a problem hiding this comment.
This breaks the unit tests. Can you locally run npm run test.
| const url = urlJoin(gitlabUrl, response["full_path"]); | |
| const url = urlJoin(gitlabUrl, response.full_path); |
In ln. 177 we are already building the url for the assets using urlJoin(gitlabUrl, repoId, url).
There was a problem hiding this comment.
see 1c48f54#diff-c8bd325917d181608f40adecdcec3c76acde134cc15a9e4a0d18636698e4fe55R152-R153
matching the previously used style
aldenbe
force-pushed
the
master
branch
3 times, most recently
from
3bd8cce
to
1d4d173
Compare
test/publish.test.js
Outdated
| @@ -56,7 +56,7 @@ test.serial("Publish a release with templated path", async (t) => { | |||
| const encodedGitTag = encodeURIComponent(nextRelease.gitTag); | |||
| const generic = { path: "${env.FIXTURE}.txt", filepath: "/upload.txt" }; | |||
| const assets = [generic]; | |||
| const uploaded = { url: "/uploads/upload.txt", alt: "upload.txt" }; | |||
| const uploaded = getUploadResponse(generic.filepath, encodedRepoId); | |||
There was a problem hiding this comment.
I would prefer to keep the test cases self-contained and hardcode the upload responses like we do for most other test data.
That would also keep amount of changes minimal.
There was a problem hiding this comment.
It is my opinion that using a helper function to mimic the gitlab api endpoint responses offers the following benefits over hardcoded values.
- Increased readability. It is more clear what determines the url values passed to the simulated release endpoint which are programatically generated by gitlab and not semantic-release it also removes the implication of some variables being treated as a gitlab response which are not actually contained in response data from the gitlab markdown upload endpoint. see parameters expected in release request taken from variable uploaded not supplied by gitlab markdown upload endpoint and uploaded implied to be a response from the gitlab markdown upload endpoint
- Increased maintainability. Should gitlab change their endpoint (again) updates need only to be made to the mocked gitlab upload endpoint instead of each test case being updated individually.
- Increased reliability. Although no test case currently exists, breakages potentially caused by the gitlab upload behavior of only using the file name and ignoring the file path would be more easily detected by mimicking the gitlab endpoint response than relying on hardcoded values and expecting developers to properly account for this, especially given that as far as I can tell this behavior is not actually documented anywhere (including in gitlab's documentation).
There was a problem hiding this comment.
gitlab response handling in publish.js would also contain the unexpected variables in point 1, which makes that point both a readability and reliability concern.
There was a problem hiding this comment.
Thanks for providing your reasoning @aldenbe, it's very clear!
I do agree with you in general, but seeing that we have many other similar places where we "hardcode" responses, e.g.
Lines 181 to 182 in 4554f49
I would however be very open to accept a PR that refactors all the responses towards the style you suggest here. Would you be willing to give that a try? Also fine if you start with what you already have and we finish it for the other responses.
@JonasSchubert WDYT?
There was a problem hiding this comment.
Simplified PR explicitly addressing this issue created #755
Unfortunately I have spent too much time on this already. Feel free to take anything desired from https://github.com/aldenbe/semantic-release-gitlab/tree/gitlab-mock-upload-endpoint and use it as you please.
There was a problem hiding this comment.
Thanks again @aldenbe for your work and fix.
I agree with @fgreinacher to keep the context of PRs to one topic to avoid too many changes in one. This usually helps to review a PR. I would be happy to see you create a second PR with your changes for the tests, @aldenbe. This can indeed improve readability. I understand if you have no time for this now, but if you would like to tackle that in the future it would be appreciated.
There was a problem hiding this comment.
Thanks for your contribution @aldenbe, greatly appreciated!
The fix itself looks perfect, but I'd prefer to keep the existing test structure.
Also we have some lint issues, see https://github.com/semantic-release/gitlab/actions/runs/10120894480/job/28034084391?pr=754. You should be able to fix them via npm run lint:fix.
|
fix(assets): changed generation of gitlab release asset url when providing file assets with target project_upload to point to url for the uploaded file
#753