Repository navigation
🐛 Location-agnostic sign release - #268
knative-prow[bot] merged 2 commits into
Conversation
knative-prow
Bot
added
approved
|
/cc @psschwei |
|
Awesome work on adding the tests 👍 |
|
/hold I'd say to wait for @upodroid as this is mostly his code. |
knative-prow
Bot
added
the
do-not-merge/hold
upodroid
left a comment
upodroid
left a comment
There was a problem hiding this comment.
LGTM
We can test this by bumping the deps and rerunning the nightly. The final artifacts will be in the nightly GCS bucket
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cardil, upodroid The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
cardil
left a comment
cardil
left a comment
There was a problem hiding this comment.
This needs to be adjusted
|
/retest |
|
Infra failure /test integration-tests |
| macos_artifacts+=("${file}") | ||
| rcodesign sign "${file}" --p12-file="${APPLE_CODESIGN_KEY}" \ | ||
| --code-signature-flags=runtime \ | ||
| --p12-password-file="${APPLE_CODESIGN_PASSWORD_FILE}" |
There was a problem hiding this comment.
I'll admit I'm slightly confused on preferring while ...; done < <(command) over command | while ...; done.
The explanation about subshells didn't make sense to me.
There was a problem hiding this comment.
I don't know to what you are referring exactly. Is it https://www.shellcheck.net/wiki/SC2013 ?
It shows couple valid options on how to iterate lines from a file/pipe/stdin. All of them are equally valid.
The problem is people keep using the invalid iteration patterns. Habits from other programming languages often makes writing shell code harder, as bash is unique in many ways.
There was a problem hiding this comment.
I meant that this format:
grep -v '^ *#' < file | while IFS= read -r line
do
echo "Line: $line"
done
Seems clearer to me, because the set of things you're looping over is at the start.
The linked docs suggest that the form using process substitution (<(...)) "avoids a subshell", but I'm not sure I understand why that would be true. In any case, it feels like having the set of things to loop over at the beginning of the loop is a bit more natural.
There was a problem hiding this comment.
Both forms are equally valid. Matter of preference, I guess.
Giving things to iterate over before the loop, can be very cloudy, especially if the command is long.
Passing it to done is clear to me, and avoids long lines. Feeding data to commands with < is quite common in shell. No point, IMHO, of making bash to look like other programming languages, it isn't similar.
Your proposal has only one minor downside. Because it uses pipe, it may fail silently without stopping the script (if pipefail option isn't set ofc).
| echo "🧮 Post Notarization Checksum:" | ||
| cat checksums.txt | ||
| local macos_artifacts | ||
| declare -a macos_artifacts=() |
There was a problem hiding this comment.
Would this be better as that fancy glob-equals used above?
There was a problem hiding this comment.
I don't see any glob-equal anywhere. Could you explain it exactly?
There was a problem hiding this comment.
On line 333, you wrote:
if [[ "${file}" == *"checksums.txt" ]]; then
I wondered if that form would be better than echo | grep
There was a problem hiding this comment.
Oops, this was supposed to be on line 368:
if echo "$file" | grep -q "darwin"; then
There was a problem hiding this comment.
Yes, it may be written as
if [[ "$file" == *"darwin"* ]]; thenIDK if it's worth changing though. This form is way less known, and it's very error prone.
For example, someone could rewrite it to:
if [[ "$file" == "*darwin*" ]]; thenWhich means completely different thing, and will not work.
But, going back to your question: I'm not seeing much benefit in a rewrite, but we definitely could do it if you feel like it.
| zip_file="$(mktemp -d)/files.zip" | ||
| zip "$zip_file" -@ < <(printf "%s\n" "${macos_artifacts[@]}") | ||
| rcodesign notary-submit "$zip_file" --api-key-path="${APPLE_NOTARY_API_KEY}" --wait | ||
| true > "${checksums_file}" # Clear the checksums file |
There was a problem hiding this comment.
Should this be a comparison, or do we want to avoid any file with checksums.txt in the name?
There was a problem hiding this comment.
I don't follow, again. There's no comparison here.
This line clears the checksums file content. The true built-in has no stdout, so redirecting it to the file, clears its contents.
There was a problem hiding this comment.
What could have been done better here, is to not clear the file entirely, but replace just the signed files hashes.
We could make such amend in a follow up, IMHO.
There was a problem hiding this comment.
It looks like GitHub made a mess of the line numbers on my comments, let me find the right line.
|
/test integration-tests Infra failure: GKE cluster failed to boot |
|
/lgtm |
|
/hold cancel |
knative-prow
Bot
removed
the
do-not-merge/hold
|
Fix confirmed, see: knative-extensions/kn-plugin-event#270 (comment) /cherrypick release-1.9 |
|
@cardil: new pull request created: #271 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Changes
/kind bug
Fixes #240
Related to knative-extensions/homebrew-kn-plugins#134