ci(weaver): update network-setups fabric setupCC.sh #3680
Conversation
dnakashima
requested review from
petermetz,
takeutak,
izuru0,
jagpreetsinghsasan,
VRamakrishna,
sandeepnRES and
outSH
as code owners
dnakashima
changed the title
|
The reason why this error is that variable "CACTI_VERSION" is not correct. So this patch can get latest version of Cacti from VERSION file. |
dnakashima
changed the title
dnakashima
changed the title
dnakashima
changed the title
| @@ -4,7 +4,7 @@ | |||
|
|
|||
| directory=$(dirname $0) | |||
|
|
|||
| CACTI_VERSION=v2.0.0 | |||
| CACTI_VERSION="v$(head -n 1 $PWD/../../../../core/network/fabric-interop-cc/contracts/interop/VERSION)" | |||
There was a problem hiding this comment.
@dnakashima This seems to hardcode the behavior in way that forces people to run the script only from a specific given sub-directory.
What that means is that if I ran this script from the project root like
$ ./weaver/tests/network-setups/fabric/dev/scripts/setupCC.shThen it would crash with some error that it can't find the VERSION because the $PWD would evaluate to the project root and then your ../../../../ would just point to some random, non-existent directory path on my file-system outside of the cacti project sources.
What I recommend to fix this is to anchor the paths to the path of the script itself the way the ci.sh script does it:
SCRIPT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd )"After which you can reach the version file like "$SCRIPT_DIR/../../../../core/network/fabric-interop-cc/contracts/interop/VERSION' (or something similar) and with this way the script will function correctly regardless of which directory my shell was in at the time of the script invocation.
I wrote all this code just from memory so there might be issues with it, but please let me know what you think!
There was a problem hiding this comment.
@petermetz Thank you for your comment. I did not consider the directory where setupCC.sh is called. So I modify how to get the path of setupCC.sh.
There was a problem hiding this comment.
@dnakashima My pleasure! Thank you for the improvements!
dnakashima
force-pushed
the
update_setupcc
branch
from
a52d1a8 to
ec75a51
Compare
There was a problem hiding this comment.
@dnakashima Please correct the commit subject to appease the linter. I also recommend using the commit type build or ci or test instead of fix because fix implies a fix in the production code.
|
@dnakashima Please also squash the commits together and make sure that the commit subject and the PR title are consistent. Right now there are multiple commits and they both have invalid commit subjects.
|
dnakashima
changed the title
Signed-off-by: Daiki.Nakashima <Nakashima.Daiki@df.MitsubishiElectric.co.jp>
dnakashima
force-pushed
the
update_setupcc
branch
from
ec75a51 to
1843519
Compare
|
@petermetz Thank you for your comment again. I squashed 2 commits and modified the PR title and commit message. So please check it. |
There was a problem hiding this comment.
@dnakashima Thank you for the updates. The commit linter is still failing, please fix that as well.
dnakashima
changed the title
dnakashima
changed the title
|
@petermetz I have added "colon" and space to the title after prefix and then linter was OK. Please check it again. |
Fixes #3679
Pull Request Requirements
upstream/mainbranch and squashed into single commit to help maintainers review it more efficient and to avoid spaghetti git commit graphs that obfuscate which commit did exactly what change, when and, why.-sflag when usinggit commitcommand. You may refer to this link for more information.Character Limit
A Must Read for Beginners
For rebasing and squashing, here's a must read guide for beginners.