Skip to content

feat(CI): Enforce charts metadata backward compatibility check #33180

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

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

geido
Copy link
Member

@geido geido commented Apr 18, 2025

SUMMARY

Introducing a new workflow to check for changes on transformProps and controlPanel files for plugins. We have identified issues with changes to charts props and controls that are not backward compatible causing disruption for charts and dashboards. This is meant to be a temporary / low-effort solution to mitigate the impact going forward. A better and longer term solution would be to implement proper metadata versioning.

Why a Github workflow?

  • All plugins follow the same structure, so changes to transformProps and controlPanel often indicate that modifications to the metadata may have been introduced. While this is not always the case, adding an extra check when altering such a critical part of the codebase is a prudent measure.
  • Each chart relies on its own specific metadata. Enforcing backward compatibility checks at the chart level would require dedicated unit tests for each case. However, developers might be inclined to modify these tests to accommodate changes, rather than using them as a true validation mechanism—ultimately reducing their effectiveness.
  • Introducing a check during CI runs is a more explicit and visible way to enforce backward compatibility. It demands deliberate attention from the developer and encourages proper validation before proceeding.

If any transformProps or controlPanel get touched, CI should re-run with a label validation:backward-compatible to confirm backward-compatibility.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

CI should pass

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

Copy link

korbit-ai bot commented Apr 18, 2025

Based on your review schedule, I'll hold off on reviewing this PR until it's marked as ready for review. If you'd like me to take a look now, comment /korbit-review.

Your admin can change your review schedule in the Korbit Console

@github-actions github-actions bot added the github_actions Pull requests that update GitHub Actions code label Apr 18, 2025
@geido geido force-pushed the geido/feat/ci-metadata-backward-check branch 3 times, most recently from ac1e4cb to feea3df Compare April 18, 2025 09:34
@geido geido force-pushed the geido/feat/ci-metadata-backward-check branch from feea3df to 57da943 Compare April 18, 2025 09:37
@geido geido force-pushed the geido/feat/ci-metadata-backward-check branch from d97567c to 28af4be Compare April 18, 2025 10:07
@geido geido force-pushed the geido/feat/ci-metadata-backward-check branch from 7e7c415 to b14a387 Compare April 18, 2025 10:46
@github-actions github-actions bot removed the plugins label Apr 18, 2025
@geido geido marked this pull request as ready for review April 18, 2025 12:07
Copy link

@korbit-ai korbit-ai bot left a comment

Choose a reason for hiding this comment

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

I've completed my review and didn't find any issues.

Check out our docs on how you can make Korbit work best for you and your team.

Loving Korbit!? Share us on LinkedIn Reddit and X

Copy link
Member

@kgabryje kgabryje left a comment

Choose a reason for hiding this comment

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

Not an expert on github workflows so I'm not diving into the code, but I like the idea as a temporary solution for ensuring backward compatibility

Copy link
Member

@msyavuz msyavuz 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 with a small nit

- name: "Check for changes in critical files"
id: check_changes
run: |
CHANGED_FILES=$(git diff --name-only origin/master...HEAD)
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 this runs diff against master but the base might be different?

@rusackas
Copy link
Member

rusackas commented Apr 22, 2025

We might not need a whole new action for this. You could just add a few filenames to this file. However, that would apply to all branches, I believe, so I'm not sure if that solves the issue for you.

@geido
Copy link
Member Author

geido commented Apr 22, 2025

We might not need a whole new action for this. You could just add a few filenames to this file. However, that would apply to all branches, I believe, so I'm not sure if that solves the issue for you.

This is great. Let me change it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
github_actions Pull requests that update GitHub Actions code size/M validation:backward-compatible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants