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

utils/github/actions: add format_multiline_string method #15002

Merged

Conversation

nandahkrishna
Copy link
Member

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

Needed for Homebrew/actions#336, see @Bo98's comment:

Escaping works differently now so this doesn't work. The new way to do it is:

{name}<<{delimiter}
{value}
{delimiter}

For security reasons delimiter must be randomly generated and unique to each run (e.g. UUID v4). It should also be something that doesn't appear in the message. It may be worth having a new method in Homebrew/brew.

See https://github.com/actions/toolkit/blob/b00a9fd033f4b30f2355acd212f531ecbbb9b38f/packages/core/src/file-command.ts#L27-L47 for an example implementation in JavaScript.

@nandahkrishna nandahkrishna added the critical Critical change which should be shipped as soon as possible. label Mar 18, 2023
@nandahkrishna nandahkrishna requested a review from Bo98 March 18, 2023 14:50
@BrewTestBot
Copy link
Member

Review period skipped due to critical label.

Comment on lines +108 to +110
# Generic GitHub Actions error.
class Error < RuntimeError
end
Copy link
Member

Choose a reason for hiding this comment

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

Are you planning to use this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I used it here (line 30):

if name.include?(delimiter) || value.include?(delimiter)
  raise Error, "`name` and `value` must not contain the delimiter"
end

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I meant: do you need to rescue this anywhere? Otherwise you could just do

  raise "`name` and `value` must not contain the delimiter"

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I guess I could've just done that 🤦🏻‍♂️

Copy link
Member

Choose a reason for hiding this comment

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

Not too late to change it!

Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

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

Nice work!

@nandahkrishna nandahkrishna merged commit 27bb6ff into Homebrew:master Mar 18, 2023
23 checks passed
@nandahkrishna nandahkrishna deleted the actions-multiline-envfile branch March 18, 2023 19:31
@MikeMcQuaid
Copy link
Member

Nice work @nandahkrishna!

@msarm
Copy link

msarm commented Mar 18, 2023

@nandahkrishna - It looks like the above change is causing an issue when I try to install the homebrew on M2 Silicon.

Error=: HEAD is now at 27bb6ff Merge pull request #15002 from nandahkrishna/actions-multiline-envfile

The output indicates that the HEAD of the current branch has been updated to a merge commit that merged the changes from a pull request created by the user "nandahkrishna".


/bin/bash -c "$(curl -fsSL https://raw.githubusercontent.com/Homebrew/install/HEAD/install.sh)"

==> Checking for sudo access (which may request your password)...
==> This script will install:
/opt/homebrew/bin/brew
/opt/homebrew/share/doc/homebrew
/opt/homebrew/share/man/man1/brew.1
/opt/homebrew/share/zsh/site-functions/_brew
/opt/homebrew/etc/bash_completion.d/brew
/opt/homebrew

Press RETURN/ENTER to continue or any other key to abort:
==> /usr/bin/sudo /usr/sbin/chown -R bhawaniya:admin /opt/homebrew
==> Downloading and installing Homebrew...
HEAD is now at 27bb6ff Merge pull request #15002 from nandahkrishna/actions-multiline-envfile
Error: Failed to download https://formulae.brew.sh/api/formula.jws.json!
Error: Failure while executing; /usr/bin/env /opt/homebrew/Library/Homebrew/shims/shared/curl --disable --cookie /dev/null --globoff --user-agent Homebrew/4.0.6\ \(Macintosh\;\ arm64\ Mac\ OS\ X\ 13.2.1\)\ curl/7.86.0 --header Accept-Language:\ en --fail --progress-bar --location --remote-time --output /Users/bhawaniya/Library/Caches/Homebrew/api/formula.jws.json --compressed --speed-limit 100 --speed-time 5 --progress-bar --silent https://formulae.brew.sh/api/formula.jws.json exited with 22.

@msarm
Copy link

msarm commented Mar 18, 2023

Installs now! Thanks

@nandahkrishna
Copy link
Member Author

@msarm The error you're seeing is unrelated to this PR, it's probably a network issue. Please try using the installer again and check the Discussions page for similar issues.

@github-actions github-actions bot added the outdated PR was locked due to age label Apr 18, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
critical Critical change which should be shipped as soon as possible. outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants