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

Add CLI flag to select pipelines to execute #592

Merged
merged 1 commit into from
Jun 20, 2024

Conversation

georg-schwarz
Copy link
Member

Required for #587

@georg-schwarz georg-schwarz requested review from joluj and rhazn and removed request for rhazn and joluj June 19, 2024 16:05
@rhazn
Copy link
Contributor

rhazn commented Jun 20, 2024

Ah failed to comment on the review on Mobile, but: I think the approach is weird to make it a required parameter with a default value. I think it would be better as an optional parameter and if it's undefined, run all pipelines. But I don't mind that much, so your choice.

@joluj
Copy link
Contributor

joluj commented Jun 20, 2024

I think the approach is weird to make it a required parameter with a default value.

Isn't that often like that? The default is running all pipelines, so I think that's fine.
It saves us a bit of complexity to know that the string cannot be undefined

@georg-schwarz georg-schwarz merged commit 1f80d25 into main Jun 20, 2024
3 checks passed
@georg-schwarz georg-schwarz deleted the interpreter-pipeline-flag branch June 20, 2024 10:09
@github-actions github-actions bot locked and limited conversation to collaborators Jun 20, 2024
@rhazn
Copy link
Contributor

rhazn commented Jun 20, 2024

Yeah, that's why I don't mind as much. But it will lead to some weird errors down the line when you write "can't find a pipeline for .*" and the user is confused because they never entered that. I know that is not how it would work right now, but by giving it a default value and then using that value later on in error messages, this kind of stuff can happen.

But as I said, I think that is a real edge case and I am happy with this as well.

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

Successfully merging this pull request may close these issues.

3 participants