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

Ensure BEFORE_SCRIPT and AFTER_SCRIPT are populated correctly #123

Merged
merged 2 commits into from
Aug 22, 2022
Merged

Ensure BEFORE_SCRIPT and AFTER_SCRIPT are populated correctly #123

merged 2 commits into from
Aug 22, 2022

Conversation

internalsystemerror
Copy link
Member

@internalsystemerror internalsystemerror commented Aug 19, 2022

After discussions with @ghostwriter, and after some simple testing on this myself to confirm, this is the proposed fix to prevent the error:

/usr/local/bin/entrypoint.sh: line 147: readarray: `BEFORE_SCRIPT=xmllint --schema vendor/phpunit/phpunit/phpunit.xsd phpunit.xml.dist': not a valid identifier

Note: I'm not able to use git CLI right now, but consider this signed off.

Copy link
Member

@boesing boesing left a comment

Choose a reason for hiding this comment

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

As stated in slack:

I would avoid the whole file descriptor stuff.
Just change the code like this and we have dedicated commands.

declare -a BEFORE_SCRIPT=()
readarray -t BEFORE_SCRIPT < <(echo "${JOB}" | jq -rc '(.before_script // [])[]' )

declare -a AFTER_SCRIPT=()
readarray -t AFTER_SCRIPT < <(echo "${JOB}" | jq -rc '(.after_script // [])[]' )

There should be no before_script which provides multiline commands. As of now, we do only provide before_script via the matrix itself without the ability to read it from i.e. .laminas-ci.json and thus, that might already be fine.

Not sure if we need an extra safe-guard for multiline commands. Upstream projects already can provide .laminas-ci/pre-run.sh where they can have their super complex multiline whatever before_script logic.

Gary Lockett added 2 commits August 22, 2022 13:57
Signed-off-by: Gary Lockett <gary@creativecow.uk>
Signed-off-by: Gary Lockett <gary@creativecow.uk>
@internalsystemerror
Copy link
Member Author

Sorry for the force push, needed to signoff that earlier commit.

Thanks also to @boesing and @Xerkus for their help with this

Copy link
Member

@boesing boesing left a comment

Choose a reason for hiding this comment

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

LGTM

@boesing
Copy link
Member

boesing commented Aug 22, 2022

I'd say, 🚢

@internalsystemerror internalsystemerror merged commit 0759bbc into laminas:1.24.x Aug 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants