Skip to content

Conversation

@Pajk
Copy link
Contributor

@Pajk Pajk commented Feb 14, 2019

The plugin was failing with a multi-pipeline yaml config when one of the pipelines were excluded.

I'm testing it with Drone 1.0-rc5 and other than this it works well so far. Thanks!

@Pajk Pajk mentioned this pull request Feb 14, 2019
@microadam
Copy link
Owner

Hi @Pajk! apologies for the delay. I have only just seen this (need to fix my notifications!)

Thanks for spotting this. Could I just ask, what the .filter(pipeline => pipeline) is for? I believe there should always be a truthy value returned from the map function. However if that is not the case It would be great to see an example.

Ideally if you could possibly put some example YAML files in a comment that expose the issue, that would be great. I really need to get some unit tests in place (this was put together quite quickly) so that would allow me to cover this issue when I do.

Thanks for your input! Much appreciated

@Pajk Pajk force-pushed the fix/multiple-pipelines branch from fb8fd94 to a09943a Compare February 20, 2019 12:11
@Pajk
Copy link
Contributor Author

Pajk commented Feb 20, 2019

You're correct .filter(pipeline => pipeline) doesn't have to be there anymore. I first tried to return null in case the pipeline was to be excluded but then I ended up with modifying the trigger.

@Pajk
Copy link
Contributor Author

Pajk commented Feb 20, 2019

And without this patch it's failing in a case where you have more than one pipelines and one of them (except the last) has changeset trigger that doesn't match any changes.

@microadam
Copy link
Owner

Great! Your latest change makes perfect sense, I had thought about multiple pipelines but not properly tested them. Thanks for the fix :)

@microadam microadam merged commit b22c939 into microadam:master Feb 20, 2019
@microadam
Copy link
Owner

new version released to dockerhub with your fix :)

@Pajk Pajk deleted the fix/multiple-pipelines branch February 20, 2019 15:42
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