-
Notifications
You must be signed in to change notification settings - Fork 27
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
#99 - Allow any entity scoring (Breaking changes) #132
#99 - Allow any entity scoring (Breaking changes) #132
Conversation
Making the change backward compatible with systemEntityName
This helps building backend proxy, as it will get the namespace and kind of the entity scorecard
plugins/score-card/src/components/ScoreCardTable/ScoreCardTable.test.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for your tremendous work! And sorry for the delay with the review, I was not feeling well over the weekend. I will also try to check on the test case locally (later this wee).
Another big question is whenever we really need the backwards compatibility, as this makes the code pretty messy and complex.. how about to bump to 0.6 version with a breaking changes?
plugins/score-card/src/components/ScoreCardTable/ScoreCardTable.test.tsx
Outdated
Show resolved
Hide resolved
plugins/score-card/src/components/ScoreCardTable/ScoreCardTable.test.tsx
Show resolved
Hide resolved
plugins/score-card/src/components/ScoreCardTable/ScoreCardTable.tsx
Outdated
Show resolved
Hide resolved
Yes, if you're OK with breaking changes, it will greatly simplify the refactor. |
Will require a major version bump
I cleaned up the code and remove the deprecated types to make the code cleaner. Thanks for your feedback! |
Will be worked on in another PR
Keep same areaScores in all.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I have reviewed the last changes and it's OK.
Few last things: would you be please so kind and run also yarn changeset
increasing the minor
version and describing the breaking changes? Not sure, how this needs to be announced / added to the changeset ... I will take care about the backstage
integration and also the SharePoint PowerShell integration.
Also WDYT about the current Readme? I guess we shall change the wording from system
to entity
...or I can do this in some follow-up PR.
I'll do the changeset tomorrow.
any particular reason why it's not a major version bump? Since it's a breaking change. I'll also have another look at the README. |
Thanks!
We follow https://semver.org/ and since the package is still under development, the version starts with zero, it's
Great, thanks! |
Yes you're right, TIL! I've made the changes to the README and added a changeset. |
Co-authored-by: Jan Vilimek <[email protected]> Signed-off-by: T0RAT0RA <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, thanks!
This PR enable any entity to be scored.
the URL to fetch json files is updated to be:
{jsonDataUrl}/{entityRef.namespace}/{entityRef.kind}/{entityRef.name}.json
Key changes:
systemScore*
variables renamedentityScore*
SystemScore
,SystemScoreArea
,SystemScoreEntry
,SystemScoreExtended
EntityScore
,EntityScoreArea
,EntityScoreEntry
,EntityScoreExtended
EntityScore
has a mandatory keyentityRef: CompoundEntityRef
(replacement ofSystemEntityName
)Scoring an API
✔️ Checklist
yarn changeset
in the root)