Skip to content

Conversation

@greg-intel
Copy link
Contributor

@greg-intel greg-intel commented Jun 27, 2023

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

@greg-intel greg-intel requested a review from a team as a code owner June 27, 2023 22:41
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>
@greg-intel greg-intel force-pushed the shellcheck-check-volume-levels-2 branch from 49b6976 to 95bc7c4 Compare June 27, 2023 22:52
@marc-hb
Copy link
Collaborator

marc-hb commented Jun 28, 2023

Originally skipped CI, due to the test itself not being part of ci testing.

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:

  • remove all the skipping from the first review
  • close this duplicate ?

Or maybe the other way round.

@greg-intel
Copy link
Contributor Author

Originally skipped CI, due to the test itself not being part of ci testing.

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:

  • remove all the skipping from the first review
  • close this duplicate ?

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.
As for why, maybe it was still seeing the description rather than the commit, which I've edited to remove the skip part.

@marc-hb
Copy link
Collaborator

marc-hb commented Jun 28, 2023

which I've edited to remove the skip part.

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."

Doesn't matter if it doesn't run on Jenkins.

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.

@greg-intel
Copy link
Contributor Author

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."

That's context, explaining how this test differs from others.

@greg-intel
Copy link
Contributor Author

SOFCI TEST

@marc-hb
Copy link
Collaborator

marc-hb commented Jun 28, 2023

Doesn't matter if it doesn't run on Jenkins.

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.

SOFCI TEST

OK, now we may never know.

That's context, explaining how this test differs from others.

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.

@marc-hb marc-hb merged commit a23d969 into thesofproject:main Jun 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants