-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
build: Fix OSS build #9172
base: main
Are you sure you want to change the base?
build: Fix OSS build #9172
Conversation
Perhaps this change is not needed as OSS build was already in your tests, but now i'm confused how alpha/run.go could ever pass compilation without this? |
We do not support Encryption in OSS mode. |
@mangalaman93 yes totally understand, I'm not trying to add EE feature to OSS, and I don't want encryption. For me the default oss-install compilation failed as it tries to enable encryption on that line, i'm trying to make sure it doesnt! |
|
@mangalaman93 Correction, the make step indeed passes, but then launching an OSS dgraph alpha node gives the following without this change: So the build is fine, but run.go does not appear to have an OSS "mode" as far as I can see, and dies trying to fetch non-defined encryption keys. Maybe I am missing a runtime flag to tell draph to run without encryption, but again I cant see a way that this code could ever work as it is hard-coded to return an error. |
What is the command you are using to run zero? |
For this test Im using: |
@RJKeevil I can reproduce it now. This is the right fix for it. If you could include it along with a test so that it doesn't happen again, that would be acceptable. Thank you for filing this PR.
|
Hi @mangalaman93, ee.GetKeys gets called in 9 different places (bulk loader, live loader etc etc) so i chose so solve it in get keys instead. Before i make the change, are you sure you prefer the fix on read of this? |
@RJKeevil good point. Feel free to make changes that pass the test and add new tests for oss. I can review the PR once ready. Thanks |
@mangalaman93 im unclear on what to do with the tests, it looks like in oss mode it runs some unit tests but never e.g. actually starts an alpha. Do I e.g. add the ldbc tests to the oss script? Also note the failing load test above is nothing to do with this change, looks like some random instability unfortunately. |
@RJKeevil you are right. We need to add tests for oss build and if you can help us with that, it would be pretty helpful. Happy to merge the PR once we have at least one test. You could pick one from the existing test and figure out a way to run it for oss build. I am busy with a few other things, but I could come back to it in some time if you can't figure it out. |
Perhaps my misunderstanding, but running "make install_oss" fails on the main repo, as e.g. on line 665 of dgraph/cmd/alpha/run.go calls ee.GetKeys, which will always return an error in OSS mode. This change returns an empty struct instead of an error, allowing the build to complete.