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

Support additional setup in ruby pipelines #318

Merged
merged 43 commits into from
Sep 19, 2024
Merged

Conversation

ryanwi
Copy link
Collaborator

@ryanwi ryanwi commented Sep 18, 2024

use case is a Ruby app that also needs python setup.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@ryanwi ryanwi changed the title Ryanwi/additional setup Support additional setup in ruby pipelines Sep 18, 2024
@ryanwi ryanwi marked this pull request as ready for review September 18, 2024 15:19
@ryanwi ryanwi requested a review from a team September 18, 2024 15:19
@Null-Is-Null
Copy link
Contributor

This will work, but that use of eval doesn't feel good from a security perspective. I know impact will be limited since this is running on a runner and will only be used internally, but what if we standardized on something like a ci-bootstrap.sh script that lives in the target ruby repo that gets run if present? I'm fine with proceeding with the approach in this PR for now if that's preferable.

@ryanwi
Copy link
Collaborator Author

ryanwi commented Sep 18, 2024

use of eval

isn't that what we're already doing here? https://github.com/signalwire/actions-template/blob/main/.github/actions/test-ruby/action.yml#L121

@Null-Is-Null
Copy link
Contributor

After reviewing the lines you linked I realize we probably shouldn't be doing that there either. That's not exactly the same as an eval but is effectively executing commands passed as a string with some checks in place (seeing if the command starts with bundle). Approving for now since potential impact is limited as described in prior comments.

@ryanwi
Copy link
Collaborator Author

ryanwi commented Sep 18, 2024

That's what I thought you'd say :-)

@ryanwi ryanwi merged commit 216556f into main Sep 19, 2024
1 check passed
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.

2 participants