-
Notifications
You must be signed in to change notification settings - Fork 45
fix: add types for namespace #1021
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
base: main
Are you sure you want to change the base?
Conversation
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.
Pull Request Overview
Adds support for a namespace
parameter across CLI, configuration types, and runtime logic.
- Introduce
--namespace
CLI option ingetCommonCommandOpts
and register it incli.ts
- Extend
MonitorConfig
, runner logic, and shared types to include an optionalnamespace
field - Add tests to verify
namespace
is correctly applied for pushed monitors and runner instances
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
src/options.ts | Add --namespace option definition |
src/dsl/monitor.ts | Extend MonitorConfig type with namespace? |
src/core/runner.ts | Pass options.namespace into monitor config |
src/common_types.ts | Add namespace? to PushOptions |
src/cli.ts | Register the new namespace option in the CLI |
tests/push/monitor.test.ts | Test support for namespace in push monitors |
tests/core/runner.test.ts | Verify runner includes namespace in monitor config |
Comments suppressed due to low confidence (1)
tests/push/monitor.test.ts:527
- Add a test case to verify the default namespace when none is provided, ensuring it falls back to the project spaceId.
it('supports namespace in config', async () => {
Co-authored-by: Copilot <[email protected]>
@@ -263,6 +263,7 @@ export type PushOptions = Partial<ProjectSettings> & | |||
retestOnFailure?: MonitorConfig['retestOnFailure']; | |||
enabled?: boolean; | |||
grepOpts?: GrepOptions; | |||
namespace?: 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.
I have raised this before, this should belong inside data_stream
#697 (comment)
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.
it's already at namespace in kibana, we can't change it, though we can additionally add support for it here?
@@ -236,6 +236,10 @@ export function getCommonCommandOpts() { | |||
'--match <name>', | |||
'run/push tests with a name or tags that matches a pattern' | |||
); | |||
const namespace = createOption( |
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 am confused with this option now, Is it synthetics data stream namespace or something else?
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.
well it's data stream namespace option
fixes #697
fixes #749
Since kibana already supports it we just need to add types, support via config and cli.
example