Bug: Parameters with typeof steps are not generated correctly

[Xavientois]

Is there an existing issue for this?

• I have searched the existing issues

Current behavior

Values passed to a parameter with the post_steps parameter type are generated incorrectly.

• The builtin checkout command produces an empty object
• A custom reusable command generates a name: property

This causes the generated config to fail when trying to run the pipeline

Minimum reproduction code

https://gist.github.com/Xavientois/21bb00fc854d75d3bab19905611d47a2

Steps to reproduce

No response

Expected behavior

• Checkout would generate - checkout
• The custom command would not include the name: property

CircleCI Config SDK version

0.10.1

Node.js version

16.18.0

In which operating systems have you tested?

• macOS
• Windows
• Linux

Other

No response

[Jaryt]

Hi! Thanks for opening this issue. It looks like this is an unexpected way of implementing post-steps which is causing the problem.

The WorkflowJob constructor has the parameter arguments, then two special arguments for pre steps and post steps.

constructor(
    job: Job | OrbRef<JobParameterLiteral>,
    parameters?: Exclude<WorkflowJobParameters, 'type'>,
    pre_steps?: StepsParameter,
    post_steps?: StepsParameter,
  )

So instead of defining the pre_steps in the parameters object yourself, you pass them along in the following arguments.

new CircleCI.Workflow('example', {}, [
    new CircleCI.workflow.WorkflowJob(vfcommon.jobs['install_and_build'], {}, [
      new CircleCI.commands.Checkout(),
      new CircleCI.reusable.ReusedCommand(vfcommon.commands['notify_slack'], {
        event: 'fail',
      }),
    ]),
  ]);

I also found an unexpected output where if you have undefined parameters, the pre and post steps will not be generated. The real issue here means that any "StepsParameter" might not be generating as expected. That's a good find. I'll change the title of this issue to reflect that. (Looks like your first title was correct 😄 )

[Jaryt]

I see now that there is a pre_steps are also on the parameters object. This is likely an error, but might be the preferable way. Happy to receive some input on which is better for the end user, as I can see pros and cons to both.

[Xavientois]

In my opinion, in order to avoid confusion like this, only one or the other should be allowed, not both. Ideally, if we decide to pass pre/post-steps as separate parameters, the type system would complain if the user tries to set them inside the parameters object.

Given that, it might be simplest to remove them as separate parameters and just pass them as members of the parameters object. That way, there is only one unambiguous way to specify pre/post-steps. For that reason, I think that passing them in the parameters object is the better option, as it makes it harder for the user to do the wrong thing (like I did) and get unexpected results.

[Xavientois]

I found another manifestation of this issue when passing the built-in persist_to_workspace command in a list of steps. Added it to the linked gist above

[Xavientois]

@Jaryt Any updates on this? We are hoping to start porting some of our existing configs to the SDK, but this is a blocker.

[KyleTryon]

Hey @Xavientois 👋

Would you be willing to test out the #176 PR and let me know if this works for you?

[Xavientois]

Sure thing! Thanks!

[Xavientois]

@KyleTryon I tested it with the following code, and still saw issues. Maybe my TS setup is broken.
https://gist.github.com/Xavientois/7ea9388408896f2be515c0140ab22a1a

[KyleTryon]

Hey @Xavientois sorry for the delay, I had to attend to some work on another repo. Coming back to take a look here. Thank you for the added info!

[Xavientois]

Perfect! Thanks! If my example works on your machine, then the issue on my end was likely my TS setup, so I would consider it fixed at that point.

[KyleTryon]

Updating with comments from PR, a new PR will be created.

#176 (comment)

https://gist.github.com/Xavientois/7ea9388408896f2be515c0140ab22a1a