Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: add check_title_template to action inputs #546

Merged
merged 4 commits into from
Apr 27, 2022
Merged

fix: add check_title_template to action inputs #546

merged 4 commits into from
Apr 27, 2022

Conversation

MIJOTHY
Copy link
Contributor

@MIJOTHY MIJOTHY commented Apr 25, 2022

check_title_template is supported internally, but not exposed via the
action interface. This commit exposes it and adds an example usage to
the build job.

Relates to:
7a62880

`check_title_template` is supported internally, but not exposed via the
action interface. This commit exposes it and adds an example usage to
the `build` job.

Relates to:
7a62880
@MIJOTHY MIJOTHY marked this pull request as ready for review April 25, 2022 12:05
@mikepenz
Copy link
Owner

mikepenz commented Apr 25, 2022

Thank you very much for the PR. I am only partially available the next 14 days, so it will take me a bit longer to review.

@MIJOTHY
Copy link
Contributor Author

MIJOTHY commented Apr 27, 2022

Fixed a misplaced period in the description string in 0cbde44 after seeing the workflow failure

@mikepenz
Copy link
Owner

Given the failure: https://github.com/mikepenz/action-junit-report/runs/6192680363?check_suite_focus=true#step:4:1
The action.yml does not allowed to hold a template

@MIJOTHY
Copy link
Contributor Author

MIJOTHY commented Apr 27, 2022

Given the failure: https://github.com/mikepenz/action-junit-report/runs/6192680363?check_suite_focus=true#step:4:1 The action.yml does not allowed to hold a template

I can think of two options:

  1. Remove the template vars from the docstring
  2. Change the code to use a different templating pattern

I'm leaning towards (2), as perhaps it's more user-friendly to avoid the clash with the GHA template delimiters. Currently a user would need to escape the ${{FILE_NAME}} template in order to avoid it being interpreted as a GHA template, so usage would look like

check_title_template: ${{ '${{FILE_NAME}}' }}

whereas if we were to change the action-internal template delimiters, we could have

check_title_template: '${FILE_NAME}'

I think this wouldn't be breaking, as currently the option isn't exposed via the action in the first place. What are your thoughts?

@mikepenz
Copy link
Owner

Possibly a very different template needs to be chosen, as ${x} aligns with the template commonly used in all sorts of languages, and I'd prefer to not run into problems due to this.

Theoretically dropping the $ should resolve the issue I suppose.

E.g. changing the templates to {{FILE_NAME}}

The current template var format `${{...}}` clashes with that used by
GitHub Actions, which makes usage unwieldy. Currently users need to
escape the `${{FILE_NAME}}` template in order to avoid it being
interpreted as a GHA template, so usage looks like:
```yaml
check_title_template: ${{ '${{FILE_NAME}}' }}
```

This commit changes the template format to `{{..}}`, so usage looks
like:
```yaml
check_title_template: '{{FILE_NAME}}'
```

This is a non-breaking change, as currently the input which makes use of
template vars isn't exposed via the action.yml file.
# Since the template delimiters used internally clash with those
# used by GitHub Actions, escape them by wrapping them in a GHA
# string expression
check_title_template: ${{ '${{SUITE_NAME}} | ${{TEST_NAME}}' }}
Copy link
Owner

@mikepenz mikepenz Apr 27, 2022

Choose a reason for hiding this comment

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

Given the template change, could you also please update this part

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aha good spot, I missed that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 888386a

@mikepenz mikepenz added the fix label Apr 27, 2022
@mikepenz mikepenz merged commit 41a3188 into mikepenz:main Apr 27, 2022
@mikepenz
Copy link
Owner

Thank you so much again @MIJOTHY

@MIJOTHY MIJOTHY deleted the fix_check-title-template-param branch April 28, 2022 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants