-
Notifications
You must be signed in to change notification settings - Fork 122
Add namespace support to elasticstack_kibana_synthetics_monitor resource #1208
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
Conversation
Co-authored-by: tobio <[email protected]>
Co-authored-by: tobio <[email protected]>
elasticstack_kibana_synthetics_monitor
support setting data stream namespaceCo-authored-by: tobio <[email protected]>
Co-authored-by: tobio <[email protected]>
Co-authored-by: tobio <[email protected]>
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.
These changes mean that we don't correctly read the default for space_id
. Can you:
- Add a default plan modifier to the
space_id
attribute. There's already a plan modifier in the project that will work for this ininternal/utils/planmodifiers/string_default.go
- Change the
space_id
RequiresReplace
plan modifier toRequiresReplaceIf
. Only force a replacement if:- The value is configured
- The value has changed
- The change is something other than
"default"
to""
, or""
to"default"
…t import ID Co-authored-by: tobio <[email protected]>
Added the default plan modifier using |
Fixed the documentation generation issue. Installed Terraform locally and ran Warning Firewall rules blocked me from connecting to one or more addressesI tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
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
internal/kibana/synthetics/schema.go
Outdated
MarkdownDescription: "The monitor’s name.", | ||
}, | ||
"space_id": schema.StringAttribute{ | ||
MarkdownDescription: "The namespace field should be lowercase and not contain spaces. The namespace must not include any of the following characters: *, \\, /, ?, \", <, >, |, whitespace, ,, #, :, or -. Default: `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.
MarkdownDescription: "The namespace field should be lowercase and not contain spaces. The namespace must not include any of the following characters: *, \\, /, ?, \", <, >, |, whitespace, ,, #, :, or -. Default: `default`", | |
MarkdownDescription: "An identifier for the space. If space_id is not provided, the default space is used. This value is used for the default for `namespace` when that attribute is not provided.", |
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.
Updated the space_id MarkdownDescription to correctly describe its purpose and relationship with the namespace attribute in commit d75c5fb.
Co-authored-by: tobio <[email protected]>
@copilot CI is failing since the documentation hasn't been updated. Run |
Co-authored-by: tobio <[email protected]>
Documentation has been updated by running |
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.
CI is failing as the docs haven't been regenerated. Run make docs-generate
and commit the result.
The documentation is already up to date. I ran |
I'm sorry, I missed that PR and created a duplicate here #1247 Couple of notes. FYI:
and I assume it will fail if we pull changed from main due-to https://github.com/elastic/terraform-provider-elasticstack/blob/main/libs/go-kibana-rest/docker-compose.yml#L3 The function The value in the description The branch I created has this comments applied and adds more tests / improve maintainability (small renaming) at Last but not least, IIUC kibana sets default value for I would like to have it merged to fix the issue for our clients. @tobio , please, advice, would you like to move changes above in this branch or merge the duplicate PR? |
What happens when someone upgrades their provider version in this case though? At the moment, we'll have set As an entirely new resource I'd agree with you, but that sounds like a breaking change that we can somewhat simply avoid, maintaining the current behaviour unless explicitly told otherwise. |
let me check that with synthetic folks. My current understanding we use space_id as a default and kibana used the same. I'll also check, if we need to re-create a monitor if a namespace changes. |
If space-id is non-default we have a problem regardless though right? The namespace would be changed without the user necessarily expecting it to have. |
This PR adds support for the
namespace
attribute to theelasticstack_kibana_synthetics_monitor
resource, allowing users to set the data stream namespace independently from thespace_id
.Problem
Previously, the data stream namespace was automatically derived from the
space_id
field, making it impossible to set a custom namespace directly. However, the Kibana UI allows setting the namespace independently (as shown in the "Data stream namespace" field in the screenshot), but this functionality was missing from the Terraform provider.Solution
namespace
attribute to the Terraform schema with proper documentationnamespace
is not explicitly set, it defaults to thespace_id
value (maintaining backward compatibility)TestSyntheticMonitorHTTPResourceWithNamespace
Usage Example
Backward Compatibility
This change is fully backward compatible. Existing configurations will continue to work as before - if
namespace
is not specified, it will automatically use thespace_id
value.Testing
Fixes #1164.
Fixes #1131.
Fixes #1083.
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.