FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix: parse provided payloads before replacing templated variables by zimeg · Pull Request #449 · slackapi/slack-github-action · GitHub

/ slack-github-action Public

fix: parse provided payloads before replacing templated variables - #449

Merged
zimeg merged 8 commits into
slackapi:mainfrom
zimeg:fix-variable-parsing
Jun 10, 2025
ConversationCommits (8)ChecksFiles changed
Merged

fix: parse provided payloads before replacing templated variables#449
zimeg merged 8 commits into
slackapi:mainfrom
zimeg:fix-variable-parsing

Conversation

zimeg commented May 31, 2025

Copy link
Copy Markdown
Member

Summary

This PR parses the provided payload before replacing templated variables to avoid erroneous parsings from perhaps variables with linebreaks - fixes #427.

Requirements

zimeg added this to the 2.2 milestone May 31, 2025
zimeg self-assigned this May 31, 2025
zimeg added bug Something isn't working semver:patch labels May 31, 2025

codecov Bot commented May 31, 2025 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.86%. Comparing base (6e4ac77) to head (3a520d6).
Report is 1 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main     #449   +/-   ##
=======================================
  Coverage   99.85%   99.86%           
=======================================
  Files           7        7           
  Lines         709      722   +13     
=======================================
+ Hits          708      721   +13     
  Misses          1        1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

This was referenced May 31, 2025
Closed
Closed

zimeg commented May 31, 2025

Copy link
Copy Markdown
Member Author

📣 A proof of concept for this fix is available in the following commits:

https://github.com/zimeg/slack-sandbox/tree/f2af0c9944c3802ffce73e270332c3d02c80a012

🔍 Test this as curious with a multline commit too perhaps:

- name: Run the release candidate
  uses: zimeg/slack-github-action@91855720aaee2e23bf81dae3512204a371ca1609 # v2.1.1-rc.1
  env:
    CHANNEL_ID: ${{ secrets.SLACK_CHANNEL_ID }}
  with:
    errors: true
    method: files.uploadV2
    token: ${{ secrets.SLACK_BOT_TOKEN }}
    payload-file-path: ./uploads.json  # Example below!
    payload-templated: true
{
  "channel_id": "${{ env.CHANNEL_ID }}",
  "initial_comment": "🚀 *New build*\n*Commit message:*\n${{ github.payload.head_commit.message }}\n",
  "file": "./README.md",
  "filename": "docs-${{ github.sha }}.md"
}
$ git commit
feat: attempt the odd after pushing this multiline commit

<3

zimeg commented May 31, 2025

Copy link
Copy Markdown
Member Author

📣 This link performs the same run but with debug logs, for the kind reviewer:

WilliamBergamin approved these changes Jun 2, 2025
View reviewed changes

WilliamBergamin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Sweet 🚀 this looks like a great fix!

Left a few non blocking comments

Comment thread src/content.js
Comment on lines +153 to +154
* @param {unknown} input - The initial value of the content.
* @returns {unknown} Content with templatized variables replaced.

WilliamBergamin Jun 2, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Sweet 💯 I like the simplified arguments, makes it easier to test 🚀

zimeg Jun 2, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@WilliamBergamin Haha I started writing all possible types but also found that testing the actual behavior is more useful here 😉

Comment thread src/content.js
*/
const out = {};
for (const [k, v] of Object.entries(input)) {
out[k] = this.templatize(v);

WilliamBergamin Jun 2, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Valid use of recursion 🥇

Comment thread src/content.js
Comment thread test/content.spec.js
@@ -257,14 +295,61 @@ describe("content", () => {
mocks.fs.readFileSync

WilliamBergamin Jun 2, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

These tests are pretty sweet 💯 should we also add a case with array options?

But this might be out of scope for this project because these elements mainly appear in interactive components like static multi-selects

Example

[
	{
		"type": "section",
		"block_id": "section678",
		"text": {
			"type": "mrkdwn",
			"text": "Pick items from the list"
		},
		"accessory": {
			"action_id": "text1234",
			"type": "multi_static_select",
			"placeholder": {
				"type": "plain_text",
				"text": "Select items"
			},
			"options": [
				{
					"text": {
						"type": "plain_text",
						"text": "*this is plain_text text*"
					},
					"value": "value-0"
				},
				{
					"text": {
						"type": "plain_text",
						"text": "*this is plain_text text*"
					},
					"value": "value-2"
				}
			]
		}
	}
]

zimeg Jun 2, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@WilliamBergamin This is a fascinating suggestion! 🧠 ✨

Right now we have a blocks array but that's at the top-level and doesn't promise we recurse as expected 🤓

I hadn't thought about sending interactive blocks from a GitHub Action but I'm now so curious about how this might connect with the same app listening for events in the workspace otherwise!

zimeg Jun 2, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

A similar example is added in 5c48fcf to confirm the templated replacement behavior 🚀

zimeg and others added 2 commits June 2, 2025 13:28
chore: merge w main
5f75caa
style: prefer stronger typing of a recursed key value record pair
2d93133 
Co-authored-by: William Bergamin <william.bergamin.coen@gmail.com>
WilliamBergamin approved these changes Jun 3, 2025
View reviewed changes

WilliamBergamin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Awesome work on the array support 💯

zimeg commented Jun 10, 2025

Copy link
Copy Markdown
Member Author

@WilliamBergamin @mwbrooks Thanks both for the most helpful reviews! I'll merge this now to close the related issue and will look into release related changes next 🚢 💨

zimeg merged commit a3c435b into slackapi:main Jun 10, 2025
zimeg deleted the fix-variable-parsing branch June 10, 2025 23:59
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

mwbrooks mwbrooks approved these changes
WilliamBergamin WilliamBergamin approved these changes
hello-ashleyintech Awaiting requested review from hello-ashleyintech

Assignees

zimeg

Labels

bug Something isn't working semver:patch

Projects

None yet

Milestone

2.1.1

Development

Successfully merging this pull request may close these issues.

Templated payload variables are replaced before the payload is parsed

3 participants

Back | FazBrowse Home | New Git URL