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

ISAICP-5988-2: Allow to pass array options to 'run' & 'exec' tasks #137

Open
wants to merge 10 commits into
base: 2.x
Choose a base branch
from

Conversation

claudiu-cristea
Copy link
Contributor

ISAICP-5988

Description

Allow to pass array options to 'run' & 'exec' tasks

Copy link
Member

@pfrenssen pfrenssen left a comment

Choose a reason for hiding this comment

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

Small remark, and let's document this with an example.

tests/Tasks/CollectionFactoryTest.php Show resolved Hide resolved
@pfrenssen
Copy link
Member

Does this also work in the following case?

exclude:
  paths:
    - cache
    - vendor

commands:
  deploy:files:
    task: exec
    command: rsync
    options:
      exclude: ${exclude.paths}

This defines an "array option" dynamically. This could be very useful. Let's test if this works.

README.md Outdated Show resolved Hide resolved
@claudiu-cristea
Copy link
Contributor Author

claudiu-cristea commented Jun 26, 2020

Does this also work in the following case?

exclude:
  paths:
    - cache
    - vendor

commands:
  deploy:files:
    task: exec
    command: rsync
    options:
      exclude: ${exclude.paths}

This defines an "array option" dynamically. This could be very useful. Let's test if this works.

I'm not sure, the config part has multiple problems that cannot be fixed in 1.x due to potential BC break. I found at least 2: #138 & #135

pfrenssen
pfrenssen previously approved these changes Aug 18, 2020
@geek-merlin
Copy link

FWIW, i recently filed this upstream as consolidation/robo#1060

@geek-merlin
Copy link

Does this also work in the following case?
exclude: ${exclude.paths}

Almost certainly not. I tried stuff like that and the ${...} constuct provided by the config component has a string output, converting arrays to comma-separated strings, discarding keys.

@claudiu-cristea claudiu-cristea changed the base branch from master to 2.x April 29, 2022 07:33
@claudiu-cristea claudiu-cristea dismissed pfrenssen’s stale review April 29, 2022 07:33

The base branch was changed.

idimopoulos
idimopoulos previously approved these changes Apr 29, 2022
@claudiu-cristea
Copy link
Contributor Author

I've temporary reverted some commits because it conflicts w/ other PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants