feat: Lens runtime config #2497
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2497 +/- ##
===========================================
+ Coverage 75.60% 75.69% +0.08%
===========================================
Files 291 293 +2
Lines 28086 28078 -8
===========================================
+ Hits 21234 21251 +17
+ Misses 5496 5475 -21
+ Partials 1356 1352 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 15 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| type Option func(*db) | ||
|
|
||
| // WithACP enables access control. If path is empty then acp runs in-memory. | ||
| func WithACP(path string) Option { |
There was a problem hiding this comment.
thought: Seeing these 'new' ACP funcs was a surprise to me when reviewing, and there is not really any documentation in the PR description, and it is done in the same commit as the new Lens stuff. This forces reviewers to spend more time/energy figuring out what is existing stuff being moved, and what is new stuff being added. I suggest (in the future, not asking to to change this now) separate commits for this kind of stuff, and please be a little more explicit in the PR description as to what has been done.
lens/config.go
Outdated
| import "github.com/lens-vm/lens/host-go/engine/module" | ||
|
|
||
| // Option is a funtion that sets a config value on the lens registry. | ||
| type Option func(*lensRegistry) |
There was a problem hiding this comment.
todo: Please do not define public-public stuff (i.e. not just internal-public stuff) in the lens package. Users (or our integration tests) should not ever be referencing this package, and by adding stuff that requires the direct reference you are making the existing internal-public stuff public-public.
Please move these somewhere else, or given that they are setting private stuff, maybe have public-public funcs in client or db that call these internal-public functions, and make sure you remove the reference to this package from the integration tests.
There was a problem hiding this comment.
We should move the lens package into an internal sub-package if it's meant to be internal only.
There was a problem hiding this comment.
Yes, this has been a long running battle, not everyone wants an internal package, although as far as I know everyone agrees that only a couple of packages are public. I created an issue for a proper internal package and we can discuss it in the standup - this conversation seems to happen every other month or so.
There was a problem hiding this comment.
To clarify, as far as I know, the fact that only a couple of packages are public is not really disputed by anyone - we all agree that public stuff can only live in client etc. What is disputed is whether that public/internal split should be enforced/documented by an internal directory or not. (so please move the public stuff out of the lens package in this PR)
| size = lensPoolSize.Value() | ||
| } else { | ||
| size = DefaultPoolSize | ||
| func NewRegistry( |
There was a problem hiding this comment.
praise: The new param order makes a lot more sense, thanks for making db the first one :)
| @@ -149,15 +107,12 @@ func newDB( | |||
|
|
|||
| // apply options | |||
| for _, opt := range options { | |||
| if opt == nil { | |||
There was a problem hiding this comment.
praise: Thanks for removing this :)
Relevant issue(s)
Resolves #2496
Description
This PR adds a lens runtime option. It also includes some cleanup and additional tests.
Tasks
How has this been tested?
make testSpecify the platform(s) on which this was tested: