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.

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

feat: Add allowlist #31

Merged
merged 5 commits into from
Jul 15, 2024
Merged

feat: Add allowlist #31

merged 5 commits into from
Jul 15, 2024

Conversation

e-fisher
Copy link
Collaborator

@e-fisher e-fisher commented Jul 15, 2024

Resolves https://github.com/grafana/k6-cloud/issues/2508

  • Refactor to use ProxyData[] instead of GroupedProxyData until we actually need groups, this makes it easier to work with proxy data
  • Show allow list dialog when recording is imported to test generator

CleanShot 2024-07-15 at 14 33 24@2x

@@ -501,7 +501,7 @@ const extractCorrelationJsonBody = (
}

const extractedValue = get(
JSON.parse(response.content),
safeJsonParse(response.content),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was failing on some recording

@e-fisher e-fisher marked this pull request as ready for review July 15, 2024 11:37
Copy link
Collaborator

@going-confetti going-confetti left a comment

Choose a reason for hiding this comment

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

LGTM 🤩

>
Allowed hosts [{allowList.length}/{hosts.length}]
</Button>
{/* Radix does not unmount dialog on close, this is need to clear local state */}
Copy link
Collaborator

Choose a reason for hiding this comment

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

In future, we can create our own wrappers for components we use often to simplify the API (for example, for dropdown menus/selects) and change the behavior like in this case 🤔

Comment on lines 94 to 103
<CheckboxGroup.Root
value={selectedHosts}
onValueChange={(e) => setSelectedHosts(e)}
>
{filteredHosts.map((host) => (
<CheckboxGroup.Item value={host} key={host}>
{host}
</CheckboxGroup.Item>
))}
</CheckboxGroup.Root>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should probably have max-height + scroll

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added! I didn't go with maxHeight, but used height instead to avoid dialog resizing when searching:

CleanShot 2024-07-15 at 15 17 26@2x

Copy link
Member

@Llandy3d Llandy3d left a comment

Choose a reason for hiding this comment

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

Great work 🚀 🚀 🚀

@e-fisher
Copy link
Collaborator Author

Moved allowlist to heading:

CleanShot 2024-07-15 at 15 50 11@2x

@e-fisher e-fisher merged commit 955a168 into main Jul 15, 2024
1 check passed
@e-fisher e-fisher deleted the feat/allowlist branch July 15, 2024 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants