Skip to content

Conversation

@JordanSussman
Copy link
Collaborator

@JordanSussman JordanSussman commented Jun 1, 2022

changes

  • Updates the existing substitute tests to utilize table tests
  • Update the SubstituteSteps test to include another test case for templates

@JordanSussman JordanSussman requested a review from a team as a code owner June 1, 2022 14:29
@codecov
Copy link

codecov bot commented Jun 1, 2022

Codecov Report

Merging #653 (8405882) into master (7dbbc86) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #653   +/-   ##
=======================================
  Coverage   55.07%   55.07%           
=======================================
  Files         195      195           
  Lines       15926    15926           
=======================================
  Hits         8771     8771           
  Misses       6781     6781           
  Partials      374      374           

@wass3r
Copy link
Collaborator

wass3r commented Jun 1, 2022

looks like the test is failing :( unless that's the intent ?

@JordanSussman JordanSussman marked this pull request as draft June 1, 2022 19:45
@JordanSussman
Copy link
Collaborator Author

JordanSussman commented Jun 1, 2022

@wass3r this was originally created to showcase that the default variable substitution syntax ${var:-default} does not appear to work outside of commands. Talking with @jbrockopp we realized the following:

  • Some variables do not get set until a build runs on the worker
  • The server attempts to make best efforts to substitute all variables available to it at the time
  • If a variable isn't set, then assume it may be set by the worker so it "puts it back"

The existing behavior is fine for most use-case but if you're trying to do variable substitution for vars passed to a template, you're going have a bad time. Thinking about opening up a new issue to talk through possible changes to this behavior to allow default syntax to be used.

@JordanSussman JordanSussman changed the title chore: add another test case for substitute refactor: update substitute tests to use table syntax Jun 2, 2022
@JordanSussman JordanSussman marked this pull request as ready for review June 2, 2022 14:43
Copy link
Contributor

@jbrockopp jbrockopp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

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.

4 participants