tests: only run functional tests when source has changed #1619
Conversation
@@ -8,6 +8,8 @@ if [ -f /opt/change-go-version.sh ]; then | |||
change-go-version 1.4 | |||
fi | |||
|
|||
SRC_CHANGES=$(git diff-tree --no-commit-id --name-only -r HEAD..origin/master | grep -vE "^Documentation/" | wc -l) |
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.
Could you just exit when there are no code changes?
git diff-tree --no-commit-id --name-only -r HEAD..origin/master | grep -vqE "^Documentation/"
if [ $? != 0 ] ; then
echo No changes to source detected.
exit 0
fi
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 was thinking of adding another check for doc changes later, similar to
DOC_CHANGES=$(git diff-tree --no-commit-id --name-only -r HEAD..origin/master | grep -E "^Documentation/" | wc -l)
and then have another if clause that handles documentation changes.
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.
Fair enough.
HEAD..origin/master
might be Semaphore specific and developers might name their git-remotes differently. Do you think we should check $SEMAPHORE=true
and $CI=true
? See Semaphore environment variables
fi | ||
|
||
# Semaphore does not clean git subtrees between each build. | ||
sudo rm -rf "${BUILD_DIR}" |
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.
If you decide to do the rm -rf
only in the $CI
case, you can add a similar if
for the other rm -rf
line 75.
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.
We clone to ${BUILD_DIR} regardless, so we have to remove it regardless. The rm
from L32 is because we assume that semaphore didn't cleanup after itself. Basically, we should just check if it's there at all and remove it before cloning it.
One small thing to fix, LGTM after that. |
fi | ||
|
||
# In case it wasn't cleaned up | ||
if [ -e "${BUILD_DIR}" ]; then |
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.
this check seems redundant?
Rebase. |
SRC_CHANGES=$(git diff-tree --no-commit-id --name-only -r HEAD..origin/master | \ | ||
grep -cEv "^Documentation/") | ||
# We might not need to process the docs | ||
DOC_CHANGES=$(git diff-tree --no-commit-id --name-only -r HEAD..origin/master | \ |
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.
How about catching changes in {ROADMAP,README,CONTRIBUTING,CHANGELOG}.md? These currently will land in SRC_CHANGES, right?
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.
Good idea!
LGTM after krnowak's comments are addressed. Thanks! |
DOC_CHANGES=1 # process docs by default | ||
DOC_CHANGE_PATTERN="\ | ||
-e '^Documentation/' \ | ||
-e 'README|ROADMAP|CONTRIBUTING|CHANGELOG)|\.md' \ |
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.
Semaphore complains about the )
:
grep: Unmatched ) or \)
Can you please rebase? |
LGTM |
tests: only run functional tests when source has changed
Fixes #1513.