-
Notifications
You must be signed in to change notification settings - Fork 46
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
adding project option to integration-tests action #48
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your work on this! Please check my comment.
Besides of that, how is authentication handled when we are talking about Commerce? 🤔
@@ -12,6 +12,10 @@ inputs: | |||
description: 'Magento 2 Open Source version number' | |||
required: true | |||
default: '2.3.5-p2' | |||
project_name: | |||
description: 'Magento 2 project name' | |||
required: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be required to not break backwards compatibility, IMHO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. However, the next line includes a default
, which takes care of things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah did not know that a default
value makes a required
parameter optional 😂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup: https://docs.github.com/en/actions/creating-actions/metadata-syntax-for-github-actions#inputs It is a bit stretched, but I believe the required
is not a flag to set this required to the user input, but required for the action itself. Anyway, yeah, this.
@@ -55,7 +57,7 @@ fi | |||
echo "Configure extension source in composer" | |||
composer config --unset repo.0 | |||
composer config repositories.local-source path local-source/\* | |||
composer config repositories.foomanmirror composer https://repo-magento-mirror.fooman.co.nz/ | |||
composer config repositories.magento composer $REPOSITORY_URL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought this is an issue, but this is the default value for $REPOSITORY_URL
anyway:
test -z "${REPOSITORY_URL}" && REPOSITORY_URL="https://repo-magento-mirror.fooman.co.nz/" |
So all fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds more like a bug in the current setup (where this variable is set but not used), so glad this can be fixed.
@MateusDadalto Nice stuff. Many thanks! I don't have enterprise at hand, so I'm glad that somebody picked up on this! |
The goal of this PR is to make possible to execute integration tests with this action for projects with magento enterprise edition.
By adding a new action parameter called project_name (default value:
magento/project-community-edition
), we allow the user to setup a different project to install if necessary, like magento/project-enterprise-edition