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

Adding ability to import dataset asset with no 'targets' field like filter divider #321

Merged
merged 8 commits into from
Oct 25, 2024

Conversation

swish-gweisang
Copy link
Contributor

Currently, importing a dashboard with filter dividers will break as the import of the dataset filters expects a 'targets' field in the yaml config file. However, a filter divider, which has no target, can still be imported if the 'targets' field defaults to an empty dict.

@CLAassistant
Copy link

CLAassistant commented Oct 15, 2024

CLA assistant check
All committers have signed the CLA.

@Vitor-Avila
Copy link
Contributor

Hey @swish-gweisang,

Thank you for catching this issue and also for working on a solution. It seems the latest commit removed your actual changes. Are you looking to move forward with this contribution? Happy to review the PR 🙌 🙏

@Vitor-Avila Vitor-Avila self-requested a review October 16, 2024 03:31
@swish-gweisang
Copy link
Contributor Author

@Vitor-Avila Thanks! My bad. Added the fix back in ...

@Vitor-Avila
Copy link
Contributor

Hey @swish-gweisang,

I left a comment in the PR. Also, could you add a test with a dashboard that would have a divider in its configuration? Or alternatively modify the dashboard mock used by tests that cover this flow to have a divider? This way we can prevent a regression.

Thank you!

@Vitor-Avila
Copy link
Contributor

Hey @swish-gweisang,

I'm wondering if you had a chance to check my last message. I'm looking forward to push this one across the finish line. 🙌

Thank you!

@Vitor-Avila
Copy link
Contributor

thanks for adding the test @swish-gweisang! 🙏 I think we're only missing to resolve the conflicts and I can restart CI. Are you able to resolve the conflicts or would you like me to handle these? thanks!

@swish-gweisang
Copy link
Contributor Author

swish-gweisang commented Oct 25, 2024 via email

Copy link
Contributor

@Vitor-Avila Vitor-Avila left a comment

Choose a reason for hiding this comment

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

LGTM!

@Vitor-Avila Vitor-Avila merged commit 65b0dd9 into preset-io:main Oct 25, 2024
5 checks passed
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