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

fix(protobuf): map file paths correctly for filesystem and git configurations #272

Merged
merged 1 commit into from
Oct 24, 2021

Conversation

ghost
Copy link

@ghost ghost commented Oct 23, 2021

This fixes an issue when using imports with protobuf described in #265.

Filesystem proto configuration

When files were indexed in filesStr, they were index as the full file path instead of the cleaned up file path. This resulted in protobuf trying to go to the file system for the path, but the file path used represented is not a fully qualified path.

Git proto configuration

Git experienced a similar issue as filesystem, so I added the relevant code there as well. However, it was a little different because I would need to "remove" the baseDirectory path from the filePath instead.

I've tested from both configurations, and it seems to work for me.

@ghost ghost changed the title fix(protobuf): map files correctly fix(protobuf): map file paths correctly for filesystem and git configurations Oct 23, 2021
@weeco
Copy link
Contributor

weeco commented Oct 24, 2021

Hey @zberardo-figure ,
thanks for taking a look into this. Your suggested changes make sense to me. I just tried these changes on my local windows machine (without docker) and they seem to work. In the previous attempts it worked for Windows but not for unix based systems.

@CEikermann can you maybe give this fix a shot (after I merged to master and the docker image is built then) to see if this fixes your issue?

@weeco weeco merged commit 9a5a631 into redpanda-data:master Oct 24, 2021
@ghost ghost deleted the zach-proto-fix branch October 26, 2021 13:12
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.

None yet

2 participants