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

refactor: use openapi client for dashboard controller #1462

Merged
merged 2 commits into from
Mar 19, 2024

Conversation

theSuess
Copy link
Member

This PR is a partial implementation of #1357. It only concerns itself with the dashboard controller to set the stage for other controllers to follow suit.

Tested normal use cases with a local instance & Grafana Cloud but would appreciate some more edge use cases to explore

@theSuess
Copy link
Member Author

Looks like we need to swap out folder IDs for folder UIDs as the linter will continue to complain about deprecated fields

Should we add this to this PR as well, or ignore the linter errors for now and follow up in a separate PR?

@NissesSenap
Copy link
Collaborator

NissesSenap commented Mar 18, 2024

Good question. I'm in a cab right now so I can't look at the code. Won't changing from id to uid be a breaking change? I can't remember how we define the crd.

It sounds easiest to ignore the linter for now and create a separate issue/pr around it. Do you think it will be a big refactor to do?

@theSuess
Copy link
Member Author

From what I've gathered during switching the implementation, the ID changes would only be internal and never exposed to the user. I'll try to implement it to see how much this changes, and we can revert the commit if someone notices a way in which this might break

@theSuess theSuess force-pushed the refactor/replace-old-client-dashboards branch from d99b1c6 to 5ca933c Compare March 19, 2024 06:47
Copy link
Collaborator

@HVBE HVBE left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks!

Copy link
Collaborator

@pb82 pb82 left a comment

Choose a reason for hiding this comment

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

LGTM, just a question about the paginated API

@theSuess theSuess merged commit 1605be3 into master Mar 19, 2024
10 checks passed
@theSuess theSuess deleted the refactor/replace-old-client-dashboards branch March 19, 2024 12:41
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.

None yet

4 participants