test release rotation: factor out selection #11161
Conversation
CHANGELOG_BEGIN CHANGELOG_END
| next=$(next_in_rotation) | ||
| grep -v "$next" release/rotation > $tmp | ||
| grep "$next" release/rotation >> $tmp | ||
| mv $tmp release/rotation |
There was a problem hiding this comment.
This isolates the knowledge of comment format to the next_in_rotation function, and will not reorder comments.
ci/cron/wednesday.yml
Outdated
| NEXT_SLACK=$(awk '/^[^#]/ {print $1}' release/rotation | head -n 1) | ||
| NEXT_GH=$(awk '/^[^#]/ {print $2}' release/rotation | head -n 1) | ||
| NEXT_SLACK=$(next_in_rotation) | ||
| NEXT_GH=$(grep "$NEXT_SLACK" release/rotation | awk '{print $2}') |
There was a problem hiding this comment.
Continuing on the theme of isolating comment format knowledge to that one function. This will fail if the Slack handle appears in a comment, but I think it's fair to assume that's unlikely.
There was a problem hiding this comment.
To avoid weird error conditions due to some unforeseen clumsiness in editing the rotation file (multiple rows with the same id are is an additional thing that comes to mind) I would suggest to make a second function that does the same as next_in_rotation but prints the second field. If you want to be very clean there's probably some way to have a common behavior and leave the field selection to something specific to the function, but I think that copying and pasting the function and changing $1 to $2 is also good enough.
This would mean that we can use comments to temporarily pull out people out of the release rotation (e.g. if they are on vacation).
There was a problem hiding this comment.
Done. Since pretty much every line has changed, I think this warrants a second look. Would you mind reviewing again?
ci/cron/wednesday.yml
Outdated
| NEXT_SLACK=$(awk '/^[^#]/ {print $1}' release/rotation | head -n 1) | ||
| NEXT_GH=$(awk '/^[^#]/ {print $2}' release/rotation | head -n 1) | ||
| NEXT_SLACK=$(next_in_rotation) | ||
| NEXT_GH=$(grep "$NEXT_SLACK" release/rotation | awk '{print $2}') |
There was a problem hiding this comment.
To avoid weird error conditions due to some unforeseen clumsiness in editing the rotation file (multiple rows with the same id are is an additional thing that comes to mind) I would suggest to make a second function that does the same as next_in_rotation but prints the second field. If you want to be very clean there's probably some way to have a common behavior and leave the field selection to something specific to the function, but I think that copying and pasting the function and changing $1 to $2 is also good enough.
This would mean that we can use comments to temporarily pull out people out of the release rotation (e.g. if they are on vacation).
| @@ -148,6 +149,9 @@ steps: | |||
| echo "Setting '$1' to '$2'" | |||
| echo "##vso[task.setvariable variable=$1;isOutput=true]$2" | |||
| } | |||
| next_in_rotation() { | |||
There was a problem hiding this comment.
If you choose to have a second function to select the GitHub handle, next_in_rotation_slack and next_in_rotation_github could be two viable names to avoid confusion.
| @@ -150,7 +150,13 @@ steps: | |||
| echo "##vso[task.setvariable variable=$1;isOutput=true]$2" | |||
| } | |||
| next_in_rotation() { | |||
| awk '/^[^#]/ {print $1}' "$PROJ_DIR/release/rotation" | head -n 1 | |||
| awk '/^[^#]/ {print $0}' "$PROJ_DIR/release/rotation" | head -n 1 | |||
There was a problem hiding this comment.
| awk '/^[^#]/ {print $0}' "$PROJ_DIR/release/rotation" | head -n 1 | |
| awk '/^[^#]/' "$PROJ_DIR/release/rotation" | head -n 1 |
No need to do this, it's just for you to know in case you didn't, a pattern with no corresponding action in awk will behave like {print $0}.
There was a problem hiding this comment.
I did not know that. Thanks!
CHANGELOG_BEGIN
CHANGELOG_END