Inconsistent reporting of run commands in "review" job

[wyardley]

Orb version:

11.1.0

What happened:

I have an orb with multiple "jobs"; RC008 and RCOO9 seem to behave somewhat inconsistently with the "review" job.

RC008: All Run steps should contain a name: I have multiple jobs that have run steps without a name, but only a single one shows up, and that is in a file like the second example below where there should be 2 violations, but only one shows up.

RC009: All Run step's commands should be imported: Almost all the same items that should be flagging above also fail this check; in this case, they are reported. However, some of the additional detail seems to be missing or confusing:

(in test file review.bats, line 160)
  `exit 1' failed
File: "./src/jobs/job1.yml"
Line number: 12
---
---
File: "./src/jobs/job2.yml"
Line number: 15
---
null
---

I can't easily throw up a public demo repo, but happy to provide links to examples to Circle employees offline (I'm on the discussion forum under the same account), or provide more details if you're not able to repro it easily. The examples below are lightly sanitized versions of 2 of the files, but of course there's other stuff in the orb repo as well.

Expected behavior:

Given the following two files, the two checks should fail on both (these are also slightly sanitized versions of the ones that cause the output above):

src/jobs/job1.yml:

description: "Do some things and then `foo bar`"
executor: <<parameters.executor>>
parameters:
  executor:
    description: The name of custom executor to use
    type: executor
    default: wizzle
steps:
  - checkout
  - restore-deps
  - setup-auth
  - run: npm install --no-save
  - save-deps

src/jobs/job2.yml:

description: "Run thing1 (and, optionally, thing2) on code"
executor: <<parameters.executor>>
parameters:
  executor:
    description: The name of custom executor to use
    type: executor
    default: wizzle
  run-thing2:
    description: whether or not to run foo
    type: boolean
    default: true
steps:
  - checkout
  - restore-deps
  - run: npm run thing1
  - when:
      condition: << parameters.run-thing2 >>
      steps:
        run: npm run thing2

Additional Information:

Also, I would love to see RC008 and RC009 retained / usable, but loosened up somewhat (I just have them ignored outright for now) - I see the value of encouraging good practices and failing on missing titles for very long run commands, or failing on long / complicated shell scripts, but IMO it's a bit overkill to require a single command with no interpolation, variables, or other complexity to be turned into a shell script

i.e.,

## not getting much value out of linting this IMO
run: npm run eslint

vs

run: <<include(scripts/script_that_runs_npm_run_eslint.sh)>>

The BATS based approach kind of makes sense, but it might be cleaner to build a linter into the CLI that has somewhat more readable output.

Anyway, while I'd frequently do something like

run:
   name: Setup stuff
   command: |
      here is
      a very long set of
      commands that I don't
      really want to see all of & 2>&1

IMO, for very simple use cases like this:

run: npm run foo

is probably (in many cases) clearer and easier to understand in both the Circle web UI and in the config itself

run:
  name: Run foo on files
  command: npm run foo

So, I know the logic would be much harder to do, but would love to see something that will flag examples where these recommended practices will really help in terms of reducing complexity or increasing readability.

[KyleTryon]

Thank you @wyardley 🙇 . I have already found an issue that I think may show that it was originally falsely approving maybe everything face palm. I am realizing the complexity of using BATS here makes it difficult to test my tests, at least automatically. So I am going to do some manual testing for the time being to get this patched and think later on how to better test the tests.

And Ah! When conditionals, excellent call, we'll have to get a secondary loop going.

[KyleTryon]

IMO, for very simple use cases like this:

run:
  name: Run foo on files
  command: npm run foo

How about we don't trigger this rule until a minimum length? It would need to be somewhat arbitrary but I am thinking maybe anything over 32 characters? That's close to three times as long as your example here, which is perfectly valid.

[wyardley]

How about we don't trigger this rule until a minimum length?

Yeah, that would be a good start, esp if the length is configurable. Mustache templating might lengthen some commands slightly beyond that (at which point you might be able to argue that it should be broken out anyway I guess), but that seems somewhat reasonable.

[wyardley]

Btw, one other minor thing I noticed but didn’t file a bug on was that private GitHub source URLs can’t be verified and so that check fails with private repos.

It’s easy enough to verify private repo top level URLs with the API if you have a token in scope, but probably easier for people with private repos to ignore the test with the complexities of dealing with different VCS providers, different link URLs etc.

[KyleTryon]

How about we don't trigger this rule until a minimum length?

Yeah, that would be a good start, esp if the length is configurable. Mustache templating might lengthen some commands slightly beyond that (at which point you might be able to argue that it should be broken out anyway I guess), but that seems somewhat reasonable.

I struggle to remember the specific example but there have been situations where mustache templating inside of bash has caused issues with injecting unwanted quotations which can in rare instances break a command when used in arguments.

I believe it might be something like, injecting a number parameter. and getting quotes.

My personal bias/opinion is to not use mustache templating inside bash, but it works perfectly fine directly in YAML where we can use it to set environment variables which are more native to our scripts.

[wyardley]

My personal bias/opinion is to not use mustache templating inside bash, but it works perfectly fine directly in YAML where we can use it to set environment variables which are more native to our scripts.

Right - that's what I ended up doing in my case (based on the examples / discussion in the docs about this), though it was pretty confusing to implement esp. in the case where a parameter was type env-var-name. I could imagine some situations where you're doing more complicated conditional logic with mustache might be either harder or easier to read when done this way (in shell).

Also, does Circle have a style preference on spaces or not inside templates (<< foo >> vs <<foo>>? I keep seeing different examples, and have ended up switching back and forth, but that might be another thing worth flagging / linting / enforcing some kind of style on.

[wyardley]

@KyleTryon thanks for the quick fix

Question: after testing with these latest changes, I see the same results / failed tests in both RC008 and RC009's results

and both mention "string formatting" - is something mixed up there? Reading the code / changes in https://github.com/CircleCI-Public/orb-tools-orb/pull/131/files, maybe this is intentional? But would love it if I can check the two separately (for example, I would like to be able to not use a separate name parameter, but also have a short run line).

[KyleTryon]

ention "string formatting" - is something mixed up there? Reading the code / changes in https://github.com/CircleCI-Public/orb-tools-orb/pull/131/files, maybe this is intentional? But would love it if I can check the two separately (for example, I would like to be able to not use a separate name pa

Hey @wyardley,

This was intentional, but perhaps we have still done something incorrectly.

In both tests, there is a check for the .name and .command property respectively. In both situations, if the run step is actually of type string, then it would not have either property and so we must fail the test. Another option I considered was adding a new test specifically to ensure it was not a string, but then we would still have to decide what to do with RC009 and RC008, it did not feel right to ignore/skip the test when it is not valid.

Am I correct your config looks like this?

- run: npm install --no-save

If so, I believe the above in intended, but very open to suggestions/prs. Let us know!