-
Notifications
You must be signed in to change notification settings - Fork 1.6k
GHA: extrinsic ordering, new filter to generate a summary #3631
GHA: extrinsic ordering, new filter to generate a summary #3631
Conversation
What is an index change vs an index decrease? |
@shawntabrizi you can see a sample input here under I guess I could be more explicit indeed. Obviously the deletion of an index is something noticeable we want to spot. An index change is significant because that means a call/module will not longer be reachable at the previous index. ** Decreases** are no index decreases they spot decreasing values (and we would hopefully never see one). |
So index changes are patterns like:
whereas decreases are patterns like:
with X positive. |
@shawntabrizi I see where you saw index decrease, I can fix that if you think this is confusing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. One thing I would request is that we fix the shellcheck nits in extrinsic-ordering-filter.sh
- I'm happy to do that if you say it's ok for me to push to your branch. I'll also make a CI/CD ticket for us to add this as a check (probs a Github Action), since shellcheck really is indispensable for maintaining shell scripts. Full shellcheck output below:
% shellcheck scripts/github/extrinsic-ordering-filter.sh
In scripts/github/extrinsic-ordering-filter.sh line 4:
#!/usr/bin/env bash
^-- SC1128: The shebang must be on the first line. Delete blanks and move comments.
In scripts/github/extrinsic-ordering-filter.sh line 9:
echo "\n## Index changes\n"
^--------------------^ SC2028: echo may not expand escape sequences. Use printf.
In scripts/github/extrinsic-ordering-filter.sh line 10:
RES=$(cat $FILE | egrep -n -i 'idx:\s*([0-9]+)\s*(->)\s*([0-9]+)' | tr -s " ")
^---^ SC2086: Double quote to prevent globbing and word splitting.
^---^ SC2002: Useless cat. Consider 'cmd < file | ..' or 'cmd file | ..' instead.
^---^ SC2196: egrep is non-standard and deprecated. Use grep -E instead.
Did you mean:
RES=$(cat "$FILE" | egrep -n -i 'idx:\s*([0-9]+)\s*(->)\s*([0-9]+)' | tr -s " ")
In scripts/github/extrinsic-ordering-filter.sh line 19:
echo "\n## Deletions\n"
^----------------^ SC2028: echo may not expand escape sequences. Use printf.
In scripts/github/extrinsic-ordering-filter.sh line 20:
RES=$(cat $FILE | grep -n '\[\-\]' | tr -s " ")
^---^ SC2086: Double quote to prevent globbing and word splitting.
^---^ SC2002: Useless cat. Consider 'cmd < file | ..' or 'cmd file | ..' instead.
Did you mean:
RES=$(cat "$FILE" | grep -n '\[\-\]' | tr -s " ")
In scripts/github/extrinsic-ordering-filter.sh line 29:
echo "\n## Decreases\n"
^----------------^ SC2028: echo may not expand escape sequences. Use printf.
In scripts/github/extrinsic-ordering-filter.sh line 30:
OUT=$(cat $FILE | egrep -i -o '([0-9]+)\s*(->)\s*([0-9]+)' | awk '$1 > $3 { printf "%s;", $0 }')
^---^ SC2086: Double quote to prevent globbing and word splitting.
^---^ SC2002: Useless cat. Consider 'cmd < file | ..' or 'cmd file | ..' instead.
^---^ SC2196: egrep is non-standard and deprecated. Use grep -E instead.
Did you mean:
OUT=$(cat "$FILE" | egrep -i -o '([0-9]+)\s*(->)\s*([0-9]+)' | awk '$1 > $3 { printf "%s;", $0 }')
In scripts/github/extrinsic-ordering-filter.sh line 31:
IFS=$';' LIST=($OUT)
^--^ SC2206: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
In scripts/github/extrinsic-ordering-filter.sh line 34:
echo $line
^---^ SC2086: Double quote to prevent globbing and word splitting.
Did you mean:
echo "$line"
In scripts/github/extrinsic-ordering-filter.sh line 35:
RES="$RES\n$(cat $FILE | egrep -i -n "$line" | tr -s " ")"
^---^ SC2086: Double quote to prevent globbing and word splitting.
^---^ SC2002: Useless cat. Consider 'cmd < file | ..' or 'cmd file | ..' instead.
^---^ SC2196: egrep is non-standard and deprecated. Use grep -E instead.
Did you mean:
RES="$RES\n$(cat "$FILE" | egrep -i -n "$line" | tr -s " ")"
In scripts/github/extrinsic-ordering-filter.sh line 45:
echo "\n------------------------------ SUMMARY -------------------------------"
^-- SC2028: echo may not expand escape sequences. Use printf.
In scripts/github/extrinsic-ordering-filter.sh line 46:
echo "\n⚠️ This filter is here to help spotting changes that should be reviewed carefully."
^-- SC2028: echo may not expand escape sequences. Use printf.
In scripts/github/extrinsic-ordering-filter.sh line 47:
echo "\n⚠️ It catches only index changes, deletions and value decreases".
^-- SC2028: echo may not expand escape sequences. Use printf.
In scripts/github/extrinsic-ordering-filter.sh line 49:
find_deletions $FILE
^---^ SC2086: Double quote to prevent globbing and word splitting.
Did you mean:
find_deletions "$FILE"
In scripts/github/extrinsic-ordering-filter.sh line 50:
find_index_changes $FILE
^---^ SC2086: Double quote to prevent globbing and word splitting.
Did you mean:
find_index_changes "$FILE"
In scripts/github/extrinsic-ordering-filter.sh line 51:
find_decreases $FILE
^---^ SC2086: Double quote to prevent globbing and word splitting.
Did you mean:
find_decreases "$FILE"
In scripts/github/extrinsic-ordering-filter.sh line 52:
echo "\n----------------------------------------------------------------------\n"
^-- SC2028: echo may not expand escape sequences. Use printf.
For more information:
https://www.shellcheck.net/wiki/SC1128 -- The shebang must be on the first ...
https://www.shellcheck.net/wiki/SC2206 -- Quote to prevent word splitting/g...
https://www.shellcheck.net/wiki/SC2028 -- echo may not expand escape sequen...
I will run |
c04a4ce
to
f8ea3d2
Compare
I fixed according to the linter's suggestion but did disable one type that would make the commands way less readable and harder to test and debug. |
I will bring another fix in, as the linter's recommandations are not without consequences... |
So this linter is not amazing... it enforces double quotes but fails to recognize an escaped one, effectively inviting you to screw up your script 😞 |
b9a69d8
to
c56156f
Compare
Rebased on master |
c56156f
to
2d9691c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM :D
* master: (138 commits) Allow an Offset to Lease Periods (#3980) Bump quote from 1.0.9 to 1.0.10 (#4013) Companion for #9834 (Transaction Priority) (#3901) chore: update `builder` image (#3884) Free disputed cores before processing bitfields (#4008) Make candidate validation timeouts configurable (#4001) Add extrinsic ordering filtering (#3631) chore: ci list files that spellcheck finds (#3992) Use background tasks properly in candidate-validation (#4002) Fix unoccupied bitfields (#4004) Bump syn from 1.0.77 to 1.0.78 (#4006) Bump jsonrpsee-ws-client from 0.3.0 to 0.3.1 (#3931) fix clock drift for assignments issued before the block (#3851) Remove unoccupied bit check (#3999) bump substrate (#4000) change genesis authority set for wococo-local, revert rococo-local (#3998) ignore irrelevant approvals in logs (#3859) avoid expect, on free availability core (#3994) preserve finalized block in active leaves (#3997) some tweaks to rococo-local (#3996) ...
In #3620, a new Github Worflow was added to compare metadata between releases and catch changes that need to trigger a transaction version bump.
This PR appends a new
Summary
section where the interesting changes are highlighted (= the changes that needs special attention such as removal, index changes and indexes decreasing).A sample output can be seen here. Note that the CI run in this example was run to compare Westend and Kusama for the solely purpose to show case while working on more differences.
The summary for this (fantasy) run looks like:
------------------------------ SUMMARY -------------------------------
⚠️ This filter is here to help spotting changes that should be reviewed carefully.
⚠️ It catches only index changes, deletions and index decreases.
Deletions
14: [-] modules: Sudo, ParachainsConfiguration, ParasInclusion, ParasInherent, ParasScheduler
Index changes
27: [Identity] idx: 17 -> 25 (calls: 15, storage: 4)
28: [Recovery] idx: 18 -> 27 (calls: 9, storage: 3)
29: [Vesting] idx: 19 -> 28 (calls: 4, storage: 1)
30: [Scheduler] idx: 20 -> 29 (calls: 6, storage: 3)
31: [Proxy] idx: 22 -> 30 (calls: 10, storage: 2)
32: [Multisig] idx: 23 -> 31 (calls: 4, storage: 2)
33: [ElectionProviderMultiPhase] idx: 24 -> 37 (calls: 4, storage: 10)
36: [Paras] idx: 47 -> 56 (calls: 5, storage: 13 -> 17)
38: [Registrar] idx: 60 -> 70 (calls: 6, storage: 3)
39: [Slots] idx: 61 -> 71 (calls: 3, storage: 1)
40: [Auctions] idx: 63 -> 72 (calls: 3, storage: 4)
41: [Crowdloan] idx: 64 -> 73 (calls: 8, storage: 4)
Decreases
n/a