Release 8.23.0.0 + upgrade cardano-api to 8.45.2.0 #749
Conversation
smelc
force-pushed
the
smelc/update-cardano-api-to-8.45.2.0
branch
2 times, most recently
from
d5aced1 to
eb48ca8
Compare
smelc
requested review from
a team,
Jimbo4350,
newhoggy,
carbolymer,
palas,
kevinhammond and
disassembler
as code owners
smelc
force-pushed
the
smelc/update-cardano-api-to-8.45.2.0
branch
from
eb48ca8 to
18a5b9b
Compare
| fi | ||
| done | ||
| echo "TARGET_TAG=${{ env.TARGET_TAG }}" >> "$GITHUB_OUTPUT" |
There was a problem hiding this comment.
I feel like the above three steps could have been one step.
There was a problem hiding this comment.
Yep, right; I'll do that 👍 See #749 (comment)
| echo "Tag not yet defined, using current commit as reference." | ||
| echo "TARGET_TAG=${{ github.ref_name }}" >> "$GITHUB_ENV" | ||
| fi | ||
| if [[ $(git tag --points-at ${{ env.TARGET_TAG }} | wc -l) == "0" ]] |
There was a problem hiding this comment.
I think the env.TARGET_TAG used above may not be the same as the one set on line 84, but instead is the same one as used on line 81. Is this intended?
If the code was defined instead such that it was all one step you could define in bash TARGET_TAG= and then use it immediately and then only ad the end do the
echo "TARGET_TAG=$TARGET_TAG" >> "$GITHUB_ENV"
There was a problem hiding this comment.
Yeah probably it would be simpler to merge the three steps into one, indeed. I will do that in a follow-up PR 👍
| - name: Define FLAKE_REF | ||
| id: define_flake_ref | ||
| run: | | ||
| flake_ref="github:${{ github.repository }}/${{ env.TARGET_TAG }}" |
There was a problem hiding this comment.
Does using $TARGET_TAG not work? Why do we need to use ${{ env.TARGET_TAG }}?
There was a problem hiding this comment.
For consistency, I've used env.TARGET_TAG everywhere. It's safer when moving steps around.
| id: define_flake_ref | ||
| run: | | ||
| flake_ref="github:${{ github.repository }}/${{ env.TARGET_TAG }}" | ||
| echo "FLAKE_REF=$flake_ref" >> "$GITHUB_ENV" |
There was a problem hiding this comment.
We seem to never use env.FLAKE_REF. Why define it?
There was a problem hiding this comment.
Good catch, I'll remove it in a follow-up PR 👍
| # (see https://stackoverflow.com/questions/22101778/how-to-preserve-line-breaks-when-storing-command-output-to-a-variable) | ||
|
|
||
| # shellcheck disable=SC2126 | ||
| nb_failure=$(echo "$conclusion" | grep "^failure" | wc -l) |
There was a problem hiding this comment.
Where does nb abbreviation come from?
There was a problem hiding this comment.
@carbolymer> it means number. I can expand it in a follow-up PR. I'm never a fan of abbreviation, even though I authored this one 😅
There was a problem hiding this comment.
But it does not in english. 😄 Can we use english abbreviation or full word?
cardano-cli/CHANGELOG.md
Outdated
| (improvement, test) | ||
| [PR 714](https://github.com/IntersectMBO/cardano-cli/pull/714) | ||
|
|
||
| - Provide a default value for `calculate-min-fee --reference-script-size` |
There was a problem hiding this comment.
Putting the default value here would be useful.
There was a problem hiding this comment.
Changed to Defaulted calculate-min-fee --reference-script-size's value to 0`
…ry the tag succeed
…GITHUB_OUTPUT, whereas it's required for the last job
smelc
force-pushed
the
smelc/update-cardano-api-to-8.45.2.0
branch
from
62c3e71 to
109b79b
Compare
Changelog
Context
How to trust this PR (for the CI change)
When the release pipeline ran on the tagged commit here:
It failed silently during the first steps, because the repo was not checked out yet:
If you go to a later run (albeit triggered manually, because the HEAD wasn't the tag anymore), you can see the same step not failing anymore: