-
Notifications
You must be signed in to change notification settings - Fork 59
check-volume-levels.sh: Shellcheck fixes. #1070
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
check-volume-levels.sh: Shellcheck fixes. #1070
Conversation
Originally skipped CI, due to the test itself not being part of ci testing.
The test doesn't pass consistently, even run in isolation, as it fails on
volume level checks.
Three instances of...
SC2295 (info): Expansions inside ${..} need to be quoted separately,
otherwise they match as patterns.
Added quotes to $search in this line:
CAP_SWITCH=${tmp#*$search} --> CAP_SWITCH=${tmp#*$search}#*"$search"}
Tested/verified on jf-adlp-rvp-nocodec-8
Signed-off-by: Greg Galloway <greg.galloway@intel.com>
49b6976 to
95bc7c4
Compare
Please don't leave temporary stuff like this in commit messages. So how come this is still not running jenkins? This still very confusing. Would it be possible to simplify everything like this:
Or maybe the other way round. |
Doesn't matter if it doesn't run on Jenkins. The file that was changed isn't part of the PR testing. |
Thanks but this is still in the commit message of the current 95bc7c4 and it does not belong there: "Originally skipped CI, due to the test itself not being part of ci testing."
The reason it matters is because I don't want to have to even think about "SKIP whatever" in the context of this particular PR; we already spent way too much time on that. Neither in the commit message, nor in the PR description nor in the test results. Just the most simple possible PR now please. |
That's context, explaining how this test differs from others. |
|
SOFCI TEST |
Wait, the other reason it matters is "why?". Why is Jenkins still skipping this after you removed all "skip" keywords? Is this expected? Seems wrong. This is off-topic (hence a simpler PR to avoid these questions) but it should really worry the Jenkins owner.
OK, now we may never know.
Fair enough, something like "not currently tested in CI" is fine. I think it's the "Originally" that I don't get and I don't like. |
Three instances of...
SC2295 (info): Expansions inside ${..} need to be quoted separately, otherwise they match as patterns.
Added quotes to $search in this line:
CAP_SWITCH=${tmp#*$search}-->CAP_SWITCH=${tmp#*"$search"}Tested/verified on jf-adlp-rvp-nocodec-8