Skip to content

Introduce before_script which will be executed prior the command #102

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

Merged
merged 4 commits into from
Jul 19, 2022

Conversation

boesing
Copy link
Member

@boesing boesing commented Jul 10, 2022

Q A
Documentation yes
New Feature yes

Description

This adds the possibility to pass gatekeeping commands to the action without actually polluting the real command.

All these quirks could be passed via that new JOB property instead of passing consecutive commands via the command property.


I am open to rename the property, I simply did not found a better naming (yet).

@boesing boesing force-pushed the feature/gatekeeper-commands branch from d6b6001 to 9c920cc Compare July 10, 2022 21:36
@Ocramius
Copy link
Member

Is this some sort of before_script:? The name "gatekeeper" kinda implies that they stop something 🤔

@boesing
Copy link
Member Author

boesing commented Jul 10, 2022

They might stop something in case they fail.

We do have that .laminas-ci/pre_run.sh which can be created in every project.
Thats kinda similar logic and might stop the command from being executed (depending on how that script is configured).

@boesing
Copy link
Member Author

boesing commented Jul 10, 2022

So lets take the xmllint example from the link in the initial post, in case the linting fails - the command is not being executed.

So instead someone has to pass xmllint ... && whateverUsesTheLintedXmlConfig, we just have an additional entry in that new introduced property.

@Ocramius
Copy link
Member

We do have that .laminas-ci/pre_run.sh which can be created in every project.
Thats kinda similar logic and might stop the command from being executed (depending on how that script is configured).

I don't fully understand how this new syntax is different, and whether it's supposed to replace the old one (to be deprecated, I suppose?) 🤔

@boesing
Copy link
Member Author

boesing commented Jul 12, 2022

I don't fully understand how this new syntax is different, and whether it's supposed to replace the old one (to be deprecated, I suppose?) 🤔

What syntax? Not sure if we are talking about the same thing.

  1. we still support pre-run.sh since that is a script which can be created by projects using our CI action, no plans to drop that
  2. we/I want to add a new way to provide additional commands to be executed via the matrix action

So the latter would be stuff like:

  1. linting of tool configurations (such as xmllint for psalm.xml/psalm.xml.dist/phpunit.xml/phpunit.xml.dist and yamllint for whatever YAM configuration, etc.) - see Use XmlLint against tool configurations (PHPUnit, PHPCS, Psalm) laminas-ci-matrix-action#31
  2. whatever else could be added as a command which needs to run beforehand which we are aware of in the matrix already

The current way of doing so would be polluting the JOB JSON by doing:

{"...": "...", "command": "xmllint psalm.xml.dist && vendor/bin/psalm --shepherd --no-cache --whatever"}

IMHO, this is not really readable and thus I would prefer something like:

{"...": "...", "command": "vendor/bin/psalm --shepherd --no-cache --whatever", "gatekeeper_commands": ["xmllint psalm.xml.dist"]}

(Still open to rename the property tho)

@Ocramius
Copy link
Member

So, if I understand it correctly, gatekeeper_commands (to be renamed, indeed) is a better way of writing bash -c "$GATEKEEPER_COMMAND" && bash -c "$CI_COMMAND"?

Looking good, just probably better to call it before_script (like in https://docs.gitlab.com/ee/ci/yaml/#before_script)

@boesing
Copy link
Member Author

boesing commented Jul 12, 2022

[...] is a better way of writing bash -c "$GATEKEEPER_COMMAND" && bash -c "$CI_COMMAND"?

Exactly. Not sure if this is really necessary tho but given the fact that we do have some stuff depending on the commands name (BC compatibility for exclusion logic), adding additional stuff to the command property might become a burden when it comes to that exclusion logic.

call it before_script

Sounds good, will adjust the code when I find some time for it. Thanks!

@boesing boesing changed the title Introduce gatekeeper commands which will be executed prior the command Introduce before_script which will be executed prior the command Jul 18, 2022
boesing added 2 commits July 19, 2022 00:27
Gitlab uses `before_script` as well and therefore, using this to keep some kind of familiar naming seems legit.

Signed-off-by: Maximilian Bösing <[email protected]>
@boesing boesing force-pushed the feature/gatekeeper-commands branch from a96d3cb to c56cd9e Compare July 18, 2022 22:38
…itted when `before_script` is being passed

Signed-off-by: Maximilian Bösing <[email protected]>
@boesing
Copy link
Member Author

boesing commented Jul 18, 2022

I've renamed gatekeeper_commands to before_script and extended the README to outline the idea of before_script.

I also created #104 to have an additional after_script. I am thinking about moving some quirks to the matrix action after we released a container version which supports before_script and after_script:

if [[ "${COMMAND}" =~ phpunit ]];then
echo "Setting up PHPUnit problem matcher"
cp /etc/laminas-ci/problem-matcher/phpunit.json "$(pwd)/phpunit.json"
echo "::add-matcher::phpunit.json"
fi

if [[ "${COMMAND}" =~ markdownlint ]];then
echo "Setting up markdownlint problem matcher"
cp /etc/laminas-ci/problem-matcher/markdownlint.json "$(pwd)/markdownlint-matcher.json"
echo "::add-matcher::markdownlint-matcher.json"
if [ ! -f ".markdownlint.json" ];then
echo "Installing markdownlint configuration"
cp /etc/laminas-ci/markdownlint.json .markdownlint.json
fi
fi

Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Thanks @boesing

@Ocramius Ocramius self-assigned this Jul 19, 2022
@Ocramius Ocramius merged commit 85fccc7 into laminas:1.22.x Jul 19, 2022
@boesing boesing deleted the feature/gatekeeper-commands branch July 19, 2022 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants