oxidized
oxidized copied to clipboard
ytti
Added support for multiple outputs
ytti/oxidized#425
Pretty much implemented as per the pointers provided in the issue, but with some minor modifications to make it work. I also had to modify lib/oxidized/nodes.rb to make api calls work. Since you wouldnt want the api to, for exampel, 'fetch' from multiple outputs I just made it so it uses the first output in the array. So if you use multiple outputs, the first one you state in the 'defaults:' line will be used by the api.
Thanks, looks good, I want someone else to ACK it too. I recall there are some talk about this feature in issues earlier, so you might want to ping them, to get review.
This has been reviewed by @davama ( se ytti/oxidized#425 ) and approved. Ok to merge?
I think I want @ElvinEfendi to chime in too.
I think it might be good. But to be honest I don't understand git+file output, but maybe git+http or file+http are reasonable combinations.
And two people separately have wanted this. I'm leaning towards merge, just want another opinion.
I'd really like to keep the core as simple as possible. This sounds like a great feature to be implemented as a hook. That being said I'll review the code in case you want to merge.
@ytti
If this should be done via hooks, then maybe also the primary output should be hook instead, as to avoid special case.
If this should be done via hooks, then maybe also the primary output should be hook instead, as to avoid special case.
Having more consistent way of configuring outputs would probably better. However my assumption! is that the case where a user wants multiple outputs is already a special case, so why not use a special hook.
probably not, there is merge conflict and likely would be refactored anyhow. I'm not gonna work on it until core rewrite, which may or may not come.
I'm not gonna work on it until core rewrite, which may or may not come.
I too am hoping for core rewrite for another request... https://github.com/ytti/oxidized/pull/654#issuecomment-271000463
hopefully it does :D
This seems to be a popular requested feature.
I realise we have different opinions on how this should be approached but would it not make sense to merge this as is (once the conflicts are fixed) and work to a 1.0 release where hooks deal with the outputs?
@wiad would you be able to rebase this?
I'm havent upgraded oxidized since I made this patch since I didnt want to break its functionality. So I'm not at all familiar with the current code base plus I'm pretty swamped right now so a new patch could take a while. So if anyone else want to fix this they are more than welcome.
