-
Notifications
You must be signed in to change notification settings - Fork 2
ENH Check if there are no actual changes to release #7
ENH Check if there are no actual changes to release #7
Conversation
e38d4e5
to
55eeaa4
Compare
action.yml
Outdated
STRING_A_COUNT=$(cat __compare.html | grep -c "<strong>$LATEST_TAG</strong> and") || true | ||
STRING_B_COUNT=$(cat __compare.html | grep -c "<strong>$GITHUB_SHA</strong> are identical.") || true |
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.
Why are we counting these separately, instead of explicitly checking for the entire "<strong>$LATEST_TAG</strong> and <strong>$GITHUB_SHA</strong> are identical."
?
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.
Because there's a line break in between them with a load of leading spaces for the second string
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.
I think this is too flimsy. They could add or remove whitespace at any time without affecting the way it renders in the browser - or change the HTML tags they're using (<strong>
could swap out for a <span>
with some css classes to handle the bold, for example). Might be better to do a quick php -a
with strip_tags
and do a whitespace-agnostic regex.
action.yml
Outdated
STRING_A_COUNT=$(cat __compare.html | grep -c "<strong>$LATEST_TAG</strong> and") || true | ||
STRING_B_COUNT=$(cat __compare.html | grep -c "<strong>$GITHUB_SHA</strong> are identical.") || true |
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.
I think this is too flimsy. They could add or remove whitespace at any time without affecting the way it renders in the browser - or change the HTML tags they're using (<strong>
could swap out for a <span>
with some css classes to handle the bold, for example). Might be better to do a quick php -a
with strip_tags
and do a whitespace-agnostic regex.
55eeaa4
to
4e433fc
Compare
@GuySartorelli updated Test run works as expected with code changes - https://github.com/emteknetnz/silverstripe-contentreview/actions/runs/6032187219/job/16367088876 |
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.
Issue #6
Example of it still releasing:
Example of it not releasing because identical: