Update CLI to not reformat input files

[ericcornelissen]

Closes #153

Adjust the implementation of the CLI to leverage the actions output list from the main library instead of the workflow output to rewrite the input file. In particular, this constructs are regular expression to find uses: directives to replace and replaces them (with a fixed format). The result is that the input file isn't reformatted, apart from the uses: directives.

Currently this is implemented in for the CLI only. It might make sense to lift this into the main library instead and use this rewrite strategy for the workflow output instead of using workflow.toString().

Also note that this duplicates the logic for the default comment from replaceActions.js. This should probably be addressed before this is merged, but I'm unsure how to resolve that within the current project structure.

[lucasgonze]

Let's brainstorm on how a regex could backfire, given the prevailing wisdom on not parsing subtle stuff as strings.

Thinking in terms of test cases would help.

[ericcornelissen]

Fair enough @lucasgonze. First of all, if you have a simpler alternative I'm more than happy to use it. Some arguments/thoughs in favor of using regular expressions:

• We know the input is valid YAML because we've already parsed it as YAML. I think it's also safe to assume the input is a file that GitHub Actions can work with (if it isn't I wouldn't expect this program to work properly).
• The motivation for not just serializing the updated data to YAML seems clear to me from #153, though it might also be reasonable to support both approaches and let the user choose.
• Simple substring-based replacement won't do because there's no restrictions on whitespace in YAML files (and I don't think it's reasonable to require users to use a specific amount of whitespace in their workflows).
• This approach (and others, like substring-based replacement) may replace something that shouldn't have been replaced (trivial example: key: uses: foo/bar@baz). I would argue that we can generally expect workflows/manifests not to contain such strings. To minimize this risk I intentionally included the uses:(\\s*) prefix.
• I believe the syntax for uses: is simple/regular enough to be parsed with a regular expressions. Though admittedly, GitHub Actions might be more permissive than I'm thinking.
• I would argue the regular expression isn't very complex, it's mostly used for whitespace matching. That being said, it may be possible to simplify the expression (though what one considers simpler depends on which aspects of the RegExp syntax you're familiar with)
  • A little more about the RegExp: The first (\\s*) is forced because there is no restriction on the amount of whitespace after the :. The group after ${actionId}@${currentVersion} is forced because we may need to add a comment to an action that doesn't have it. This group needs to match a comment, which might be preceded by whitespace (so the second (\\s*) is forced too). The remaining part is for matching the line comment text, so I opted for the most general expression (anything that's not a line-ending) to match and replace it (which is the same as the existing behavior).
• ReDoS might be a concern for some users (though in most cases I would argue the input is trusted). But this expression is simple enough to avoid ReDoS vulnerabilities, and ReDoS detector agrees.
• As long as there's concerns I'm more then happy to put this behind an opt-in experimental flag like --experimental-replace-inline - the best way to test is on real world data.

Some example cases to consider (excluding combinations of scenarios):

# with leading hyphen
- uses: foo/bar@baz

# without leading hyphen
- name: name
  uses: foo/bar@baz

# one space after uses
- uses: foo/bar@baz

# with pin comment, no spacing
- uses: foo/bar@baz#pin@main

# with pin comment, spacing before #
- uses: foo/bar@baz #pin@main

# with pin comment, spacing after #
- uses: foo/bar@baz# pin@main

# with pin comment, spacing before and after #
- uses: foo/bar@baz # pin@main

# with non-pin comment
- uses: foo/bar@baz # foobar

Some cases the current expression doesn't handle (should probably be fixed, only realized while writing this response):

• whitespace before colon

  - uses : foo/bar@baz

• no final newline:

  - uses: foo/bar@baz
  # No newline at end of file

[ericcornelissen]

Some cases the current expression doesn't handle (should probably be fixed, only realized while writing this response):

These, and one more case, are now handled with e28bbefa1651bf688ec9c2e8e3f1540224bf70e2

[mheap]

@ericcornelissen I think this is a good addition. For it to be accepted, it would need to be refactored in to replaceActions (https://github.com/mheap/pin-github-action/blob/main/replaceActions.js) and tests added

[ericcornelissen]

@mheap, thanks! Before I do that, do you want me to replace the existing implementation or add it as an alternative strategy? And in case of the latter, any preferences for the API?

[mheap]

I’d be ok replacing the implementation so long as all of the tests still pass.On 17 Sep 2024, at 18:20, Eric Cornelissen @.***> wrote: @mheap, thanks! Before I do that, do you want me to replace the existing implementation or add it as an alternative strategy? And in case of the latter, any preferences for the API?

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>

[ericcornelissen]

@mheap I updated the PR now. Looking forward to your review :slightly_smiling_face:

I replaced the existing implementation for the new one. The test suite for replaceActions still passed (after adjusting for the changed API). I expanded it with the cases I have mentioned in this thread and two more related to indentation.

Also, this change eliminated the need for the --yaml-line-width and --yaml-null-str inputs but I kept them on the CLI to avoid breaking users that expect those flags to exist - I just changed the help text to mark them as deprecated.