Fix releasing #2707
Fix releasing #2707
Conversation
hasufell
requested review from
jneira,
Anton-Latukha and
Ailrun
as code owners
hasufell
force-pushed
the
gitlab-release-pull
branch
from
b169764 to
5e77f31
Compare
|
sorry dont have time to review the pr, surely some other maintainer(s) could do it |
|
Currently reviewed it 2 times (had 2 passes). Understood the subject matter & read through the code. I am not adept in AWK language. Honestly - to understand PR better structurally (where what how goes) & run inference inside - the third & maybe fourth passes are needed. Of course that it is a single commit with a big Bourne shell diff. I understand - this particular subject matter may make separate commits difficult, but the form also becomes harder to review & internalize what was done precisely & the meaning of it. It is a long way of saying - to give a proper review - I need another pass. Seems pretty straightforward & understandable, though. Seen that you've done testing during the work process. I can review better & say/not say something more meaningful later in the day. |
That's just some foo that I might remove later, because the job names on gitlab differ from the tarball names (and we need both for download via API). That can be equalized, but requires another round of gitlab CI, which I wanted to skip for testing.
Yeah, we just drop the static builds from github CI, pull all artifacts from gitlab and then upload them to the github release. We use this rather convoluted github |
|
No. It is mine issue. I just as many was postponing learning AWK for years. At this time & level I probably need to know some AWK. Yes, I understood that it is just reshuffling of fields, between probably job & the tarball names. |
Thank you for complete clarification. As upload-release-asset action mentions was https://github.com/softprops/action-gh-release considered instead? It may be looked into after the PR merge. |
|
The link to the resulting workflow run is: https://github.com/hasufell/haskell-language-server/runs/5169934711?check_suite_focus=true |
| if [[ "${{ matrix.target-os }}" == "Windows" ]]; then | ||
| 7z x "*.zip" | ||
| rm *.zip | ||
| set -eux |
There was a problem hiding this comment.
Oh.
Thank you for setting the x here, great idea.
There was a problem hiding this comment.
Reviewed.
The situation & solution is understandable and straightforward.
Only had a couple of the most minimal improvements to the code, but the code is overall of high quality.
The use of the set -x in the shell script would help when some naming changes or API changes. I think for the prolonged shell parts it is a good idea to keep the x flag on (which it is currently).
With that https://github.com/hasufell/haskell-language-server/runs/5169934711?check_suite_focus=true works & artifacts are uploaded - I think passing my review is enough to mandate the merge.
|
[skip circleci] |
|
@hasufell, should we report to evaluate use of https://github.com/softprops/action-gh-release later, or it was considered/does not implement the requirements. |
Afaiu that action creates a release, which we don't want. We create a release and trigger a build instead. I don't think it's worth investigating that action. |
|
Thank you @hasufell for this. |
This reverts commit 34e9914.
Tested here: https://github.com/hasufell/haskell-language-server/releases
The idea here is that we pull the archives from the gitlab API instead of implementing pushing. That means a pipeline with the same tag as the release will have to have succeeded fully on gitlab CI.
Related: #2663