Skip to content
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.

Sign up for GitHub

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

Expand configuration_links to support internal/external links #844

Merged
merged 14 commits into from
Dec 13, 2024
Merged

Expand configuration_links to support internal/external links #844

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
1e3c65b
Expand configuration_links to support internal/external links
criamico Dec 12, 2024
54ff32b
Fix changelogs and improve tests
criamico Dec 12, 2024
0326728
Fix tests
criamico Dec 12, 2024
e258a24
Fix test again
criamico Dec 12, 2024
678fa4b
Use kbn: format instead of additional fields
criamico Dec 12, 2024
d1ee4b1
fix tests
criamico Dec 12, 2024
f5cc7b1
fix missing comma
criamico Dec 12, 2024
fa99813
Remove uncorrect test
criamico Dec 12, 2024
e941595
Address code review
criamico Dec 12, 2024
ae17e94
fix tests again
criamico Dec 12, 2024
e4a8259
update pattern
criamico Dec 12, 2024
6112482
fix pattern in tests
criamico Dec 12, 2024
cf1cb93
remove oneOf
criamico Dec 13, 2024
12d648c
Fix description
criamico Dec 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions code/go/pkg/validator/validator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,7 @@ func TestValidateFile(t *testing.T) {
[]string{
"field policy_templates.0.configuration_links: Array must have at least 1 items",
"field policy_templates.1.configuration_links.0: url is required",
"field policy_templates.1.configuration_links.1: internal is required",
},
},
}
Expand Down
3 changes: 3 additions & 0 deletions spec/changelog.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@
- description: Add support for configuration links to kibana manifest.
type: enhancement
link: https://github.com/elastic/package-spec/pull/834
- description: Expand support to internal and external configuration_links defined in kibana manifest.
type: enhancement
link: https://github.com/elastic/package-spec/pull/844
- version: 3.3.0
changes:
- description: Add support for content packages.
Expand Down
8 changes: 8 additions & 0 deletions spec/integration/manifest.spec.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,13 +225,21 @@ spec:
enum:
- action
- next_step
appId:
criamico marked this conversation as resolved.
Show resolved Hide resolved
description: The identifier of the Kibana app to navigate to. Set this field when `internal` is true.
criamico marked this conversation as resolved.
Show resolved Hide resolved
type: string
content:
description: Link description
type: string
internal:
description: Set to true for internal kibana links, false for external links. Defaults to false.
type: boolean
default: false
required:
- title
- url
- type
- internal
criamico marked this conversation as resolved.
Show resolved Hide resolved
icons:
description: List of icons for by this package.
type: array
Expand Down
14 changes: 13 additions & 1 deletion test/packages/bad_configuration_links/manifest.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,10 +79,22 @@ policy_templates:
organization: security
division: engineering
team: cloud-security
# bad format: missing fields
configuration_links:
# bad format: missing field url
- title: Security overview
type: next_step
internal: true
# bad format: missing field `internal`
- title: View Agents
url: "/agents"
type: next_step
content: "Check your agents in Fleet"
appId: "fleet"
# bad format: `internal: false` should match http or https format
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how to enforce this rule. I found the pattern field but can I enforce it conditionally (based on internal field)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you would need to use anyOf or oneOf. Something like this:

          internal:
            description: Set to true for internal kibana links, false for external links. Defaults to false.
            type: boolean
            oneOf:
              - const: true
                pattern: "^/"
              - const: false
                pattern: "^https?://"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it can be used if - then definitions here to set those conditions

I've tried to set these conditions in the manifest definition:

 $ git diff
diff --git spec/integration/manifest.spec.yml spec/integration/manifest.spec.yml
index cf39a4f..bd79026 100644
--- spec/integration/manifest.spec.yml
+++ spec/integration/manifest.spec.yml
@@ -235,6 +235,30 @@ spec:
             description: Set to true for internal kibana links, false for external links. Defaults to false.
             type: boolean
             default: false
+        allOf:
+          - if:
+              properties:
+                internal:
+                  const: true
+            then:
+              required:
+                - appId
+              properties:
+                url:
+                  type: string
+                  pattern: '^/'
+          - if:
+              properties:
+                internal:
+                  const: false
+            then:
+              not:
+                required:
+                  - appId
+              properties:
+                url:
+                  type: string
+                  pattern: '^http(s)?://'
         required:
         - title
         - url

Adding those definitions there are some errors, that probably are legit.

These conditions enforce:

  • a pattern for url depending on the internal value
  • appId is set required or not depending on the internal value.
    Is there any other condition that should be added ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I'm switching to use format kbn:... for internal links as per below discussion. Example kbn:app/integrations/browse

- title: View Agents
url: "/agents"
type: next_step
internal: false
inputs:
- type: apache/metrics
title: Collect metrics in agentless
Expand Down
18 changes: 15 additions & 3 deletions test/packages/good_v3/manifest.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,22 @@ policy_templates:
division: observability
team: obs-infraobs-integrations
configuration_links:
- title: Security overview
url: "/security/overview"
- title: View Agents
url: "/agents"
type: next_step
content: "View security overview"
content: "Check your agents in Fleet"
appId: "fleet"
internal: true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of adding the internal setting, could we use the urls prefixed by kbn that are used in many other places?

      - title: View Agents
        url: "kbn:/app/fleet/agents"
        type: next_step
        content: "Check your agents in Fleet"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't find any examples in package-spec, but maybe you refer to the Kibana dev tools?

We could use this format, it requires to do some more manipulation in kibana as the url needs to be split (it cannot be used directly by kibana this way) and it's also less explicit. However, reading your other comments I understand your concerns that this could be a bit too specific to Kibana internal way of handling urls.

Copy link
Contributor Author

@criamico criamico Dec 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the PR and the description based on this discussion.

criamico marked this conversation as resolved.
Show resolved Hide resolved
- title: Elastic website
url: "https://www.elastic.co/"
type: action
content: "See more"
internal: false
- title: Configure fleet
url: "https://www.elastic.co/guide/en/fleet/master/fleet-overview.html"
type: next_step
content: "See docs"
internal: false
inputs:
- type: apache/metrics
title: Collect metrics from Apache instances
Expand Down
5 changes: 5 additions & 0 deletions test/packages/kibana_configuration_links/changelog.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
- version: 1.0.2
changes:
- description: Expand kibana configuration links
type: enhancement
link: https://github.com/elastic/package-spec/pull/844
- version: 1.0.1
changes:
- description: Add kibana configuration links
Expand Down
25 changes: 21 additions & 4 deletions test/packages/kibana_configuration_links/manifest.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ format_version: 3.3.1
name: kibana_configuration_links
title: Kibana Configuration Links
description: Kibana configuration links examples
version: 1.0.1
version: 1.0.2
type: integration
source:
license: "Apache-2.0"
Expand Down Expand Up @@ -40,10 +40,27 @@ policy_templates:
division: observability
team: obs-infraobs-integrations
configuration_links:
- title: Security overview
url: "/security/overview"
- title: View Agents
url: "/agents"
type: next_step
content: "View security overview"
content: "Check your agents in Fleet"
appId: "fleet"
internal: true
- title: Browse integrations
url: "/browse"
type: action
appId: "integrations"
internal: true
- title: Elastic website
url: "https://www.elastic.co/"
type: action
content: "See more"
internal: false
- title: Configure fleet
url: "https://www.elastic.co/guide/en/fleet/master/fleet-overview.html"
type: next_step
content: "See docs"
internal: false
inputs:
- type: apache/metrics
title: Collect metrics from Apache instances
Expand Down