-
Notifications
You must be signed in to change notification settings - Fork 50
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
feat: Allow lens runtime selection via config #2684
feat: Allow lens runtime selection via config #2684
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2684 +/- ##
===========================================
- Coverage 78.16% 78.13% -0.03%
===========================================
Files 303 308 +5
Lines 23052 23081 +29
===========================================
+ Hits 18018 18034 +16
- Misses 3674 3680 +6
- Partials 1360 1367 +7
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 11 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
4aaae15
to
6cf51d6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I just have some minor change requests
- os: ubuntu-latest | ||
client-type: go | ||
database-type: badger-memory | ||
mutation-type: collection-save | ||
lens-type: wazero |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: didn't you mention in the description that you added wazero with Windows?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm pretty sure I updated the description yesterday once I failed to get that working in the CI - doesn't seem to have saved though. I've (re?) updated the description.
type LensRuntimeType string | ||
|
||
const ( | ||
DefaultLens LensRuntimeType = "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todo: please give it a reasonable value of add a documentation why there is only 1 value here and why it has to be empty string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do
- Doc default + map
17150d0
to
87f8e0e
Compare
The node setup is far more powerful, and the new WithRuntime func will allow selecting the runtime via the CLI
87f8e0e
to
6916237
Compare
71b5046
to
e53d161
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Nice to see the node options expanded. One small documentation todo.
@@ -126,6 +126,7 @@ func MakeStartCommand() *cobra.Command { | |||
http.WithAllowedOrigins(cfg.GetStringSlice("api.allowed-origins")...), | |||
http.WithTLSCertPath(cfg.GetString("api.pubKeyPath")), | |||
http.WithTLSKeyPath(cfg.GetString("api.privKeyPath")), | |||
node.WithLensRuntime(node.LensRuntimeType(cfg.GetString("lens.runtime"))), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todo: can you add an entry to docs/config.md
for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do, thanks Keenan!
- Have a look at config.md and add an entry
0ab2c8c
to
e53d161
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Adding these new required job names to merge the PR
Relevant issue(s)
Resolves #2683
Description
Allows lens runtime selection via config/cli param.
Also adds CI jobs to the matrix to test wasmer and wazero. I was hoping it would solve the windows build issue, but still no joy there :(