-
Notifications
You must be signed in to change notification settings - Fork 31
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
Adding OPA debugger #218
Adding OPA debugger #218
Conversation
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
…ug workspace action Signed-off-by: Johan Fylling <johan.dev@fylling.se>
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
Conflicts: src/extension.ts
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
executable: vscode.DebugAdapterExecutable | undefined, | ||
): vscode.ProviderResult<vscode.DebugAdapterDescriptor> { | ||
if (!executable) { | ||
promptForUpdateRegal(minimumSupportedRegalVersion); |
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.
Do we need to allow the user to configure another debug adapter, similar to the opa.languageServers
option for language servers?
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.
If someone comes along with a different implementation, we can accomodate that then. For now, I'd say we leave it be.
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! Just a couple of questions.
package.json
Outdated
"command": { | ||
"type": "string", | ||
"description": "The OPA command to run. E.g. 'eval', 'test'.", | ||
"default": "run" |
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.
The default is run
, as in opa run
?
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.
No, this should be eval
. It's been run
in the past, but I've since changed it to eval
as it makes more sense; the closest equivalent is opa eval
. Will change.
"items": { | ||
"type": "string" | ||
}, | ||
"default": ["${workspaceFolder}"] |
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.
Can this depend on another config attribute? If so, opa.roots
would be a good choice in order to not have users repeating that here.
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.
That should be possible to hook up, I think. We can't outright drop it and use opa.roots
in its stead, as it must be possible to opt out this and only use the dataPaths
setting. Would it make sense to allow you to specify "${opa.roots}"
, and defaulting to ["$opa.roots"]
?
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, as far as I understand the default value for opa.roots
is already ["${workspaceFolder}"]
, so if you don't configure anything, it'd mean the same thing as above, while allowing you to set this via that option
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.
Ah, so basically, if this isn't set, we use opa.roots
by default. That makes sense 👍 .
package.json
Outdated
}, | ||
"logLevel": { | ||
"type": "string", | ||
"description": "Set the log level. One of: 'debug', 'info', 'warn', 'error'." |
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.
Log level of what exactly? :)
And what'd the default?
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.
The level of logging that is returned to the vs-code debug console. To make it possible to troubleshoot debugger issues. There is no default. If not set, no logging is returned at all. I'll update the description.
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.
Thanks!
command: "eval", | ||
query: "data", | ||
bundlePaths: ["${workspaceFolder}"], | ||
enablePrint: true, |
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.
Shouldn't these values be fetched from the config? Or that's not been resolved at this point?
Same question applies below where similar objects are returned.
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.
The below objects are here to prime new launch configurations in .vscode/launch.json
with default values.
This specific object is for when you launch an ad-hoc debug session with the debug button in the top right of the editor. I suppose we could allow you to configure this through package.json
. But on the other hand, if you're not happy with the default values, you can just create a custom, names launch config in .vscode/launch.json
.
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.
Just trying to ensure that we set as smart defaults as possible, and if you've already set e.g. bundlePaths in one location (your .vscode/settings.json
) you shouldn't have to do it twice.
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.
I think if I drop bundlePaths
here, it should pan out to defaulting to whatever value you've set for opa.roots
(once that change is in).
executable: vscode.DebugAdapterExecutable | undefined, | ||
): vscode.ProviderResult<vscode.DebugAdapterDescriptor> { | ||
if (!executable) { | ||
promptForUpdateRegal(minimumSupportedRegalVersion); |
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.
If someone comes along with a different implementation, we can accomodate that then. For now, I'd say we leave it be.
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
- Inspecting the `data` and `input` documents | ||
- Inspecting local variable values | ||
- Inspecting values in the Virtual Cache (global results of rule and function evaluations) | ||
|
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.
We could also add a LSP-style gif animation here. But that lies outside of my current skill set 😄.
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.
@charlieegan3 if you get some time later today or tomorrow, can you please try this out and make a recording dor the README in the process? :) We'll want to use that for the Regal docs too!
No description provided.