Removing maxdepth option to find #296
Conversation
|
@abe545 or @ido-namely , sorry to ping you but you seem to be the two folks that could either chime on this PR or point me to someone who check this? |
Hi @eleduardo - what is the structure of your protorepo/how are you calling this command? I ask because namely definitely generates golang code for protos that are nested more than 1 level deep (I'm not as familiar with the golang side of things, but I'll try to help!) |
|
Hey @abe545 thank you so much for the reply overall it looks a lot like what the issue #251 reports more or less protos/domain1/objectX/v1/.proto We are using buf and that on itself asks us to at least have one level with the version (the vX). The usage of the image is more or less like this switching to python, C++, java, etc works fine... I did run the above with -it and overwrote the entrypoint to debug and adding a I am actually surprised that this has not happened to other folks other than the issue in #251 so also happy to close this if it is something I am doing wrong... A slightly different approach would be to add another switch for go so if you state that -l go and you do not pass this other switch the depth behavior remains but if it is set then the max depth is not added? (therefore keeping the current behavior in place but have the ability to override) Let me know! thank you |
|
@eleduardo I think I might be spotting the main difference. Have you tried passing the Having said that, we seem to keep our directory structure fairly shallow, and when we generate the protos, we are explicit about the relative path to the folder that contains the service definition. If the include doesn't work for you, I'd support adding an opt-in option to make it work the way you need. I'd be hesitant to change the existing behavior by default, though, because it is working for us (and hopefully others). |
|
@abe545 thanks again for the explanation and input. The -i sort of works but makes the go gen a bit more complex and it does not follow the other languages... I did add an extra flag to turn off the depth so the default is to keep the depth the way it has always worked but it can now be overridden I am ignoring the codacy advice on this one since I don't think I want to quote the max depth in this case. Let me know if you see any issues with this approach. I tested that not passing the switch keeps the current behavior and I did test go and python generation |
Hi @eleduardo , thank you for the PR. I believe that this is no longer an issue (otherwise I would expect you would get the error I support the approach of adding the flag and keeping the default behavior since we would want to avoid generating unexpected files when aiming to generate only the files that are 1 level deep. You PR LGTM. Could you please try to add test cases with the flag on/off in https://github.com/namely/docker-protoc/blob/master/all/test.sh? |
|
Hi @eleduardo, is this still something you want to pursue? |
|
Hi @ido-namely yes! Sorry I have not had the time to add the tests. I was looking at them and it seemed to me that there are no tests for testing the -d paths but I can whisk something over the weekend if that is ok |
of course! thank you! |
|
@ido-namely sorry for the huge delay! things got super busy. I did spruce the tests but what I noticed is that the -d was never actually tested. Unfortunately it seemed that the way the protos were setup had issues with -d so I changed the import paths a bit to be able to test it. I added a loop for testing -d for all the languages and I also added a test case with the depth change. Let me know what you think. thanks! |
There was a problem hiding this comment.
Thanks so much @eleduardo ,
Please see my comments.
Thanks!
| done | ||
|
|
||
| #Generate proto files but using the -d switch to just pass a directory input | ||
| for lang in ${LANGS[@]}; do |
There was a problem hiding this comment.
mind refactoring this a little bit and use the same loop to test all withdir true/false cases?
| done | ||
|
|
||
| #Test the -d for go but also add the the "--go-full-directory-depth" which removes the shallow find flag | ||
| testGeneration "go_full_depth_search" "go" "true" "gen/pb-go" 0 "--go-full-directory-depth" |
There was a problem hiding this comment.
- I think that for this test we should have multiple proto files in different directory depths and verify that they are all generated, and also that those files are NOT generated when the flag is not used.
- If you don't mind also moving this test to where the other
gospecific tests are?
|
@eleduardo – please, if you're still interested, refresh this PR and we'll take a fresh look at merging it. Else we can close it out. |
fixes #251
I am having the same issue that the reporter of the issue described. I tracked it down to the
findcommand with the maxdepth flag, when the directory is passed that flag makes it sofinddoes not return any proto file in the search.. then of course protoc complains that there are no files passed.Not sure why the maxdepth was set for this use case but I tested with this fix and I can generate my code correctly just like with any other language (tested go, cpp and python)