-
Notifications
You must be signed in to change notification settings - Fork 197
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
[announcements] Migrate announcements plugin #1134
Conversation
Signed-off-by: Kurt King <[email protected]>
Missing ChangesetsThe following package(s) are changed by this PR but do not have a changeset:
See CONTRIBUTING.md for more information about how to add changesets. Unexpected ChangesetsThe following changeset(s) reference packages that have not been changed in this PR:
Note that only changes that affect the published package require changesets, for example changes to tests and storybook stories do not require changesets. Changed Packages
|
Signed-off-by: Kurt King <[email protected]>
f6397c1
to
cfd2789
Compare
Signed-off-by: Kurt King <[email protected]>
Signed-off-by: Kurt King <[email protected]>
"scripts": { | ||
"dev": "concurrently \"yarn start\" \"yarn start-backend\"", | ||
"start": "yarn workspace @backstage-community/plugin-announcements start", | ||
"start-backend": "yarn workspace @backstage-community/plugin-announcements-backend start", | ||
"start:ci": "concurrently \"yarn start\" \"yarn start-backend:ci\"", | ||
"build": "backstage-cli repo build --all", | ||
"tsc": "tsc", | ||
"tsc:full": "tsc --skipLibCheck false --incremental false", | ||
"new": "backstage-cli new --scope internal", | ||
"clean": "backstage-cli repo clean", | ||
"test": "backstage-cli repo test", | ||
"test:all": "backstage-cli repo test --coverage", | ||
"lint": "backstage-cli repo lint --since origin/main", | ||
"lint:all": "backstage-cli repo lint", | ||
"version": "changeset version && yarn install && (git commit -am 'Update internal dependencies' || true)", | ||
"release": "changeset publish", | ||
"prepare": "husky install", | ||
"postinstall": "husky install || true" |
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.
todo: do I need to rework these?
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.
Remove release
and prepare
, please.
Signed-off-by: Kurt King <[email protected]>
I've thoroughly reviewed 53 files and skimmed another 30. It would be helpful to have an initial review. |
Closing and reopening for the CI issue (we're working on that in #875) |
CI seems to be failing with on the |
This PR has been automatically marked as stale because it has not had recent activity from the author. It will be closed if no further activity occurs. If the PR was closed and you want it re-opened, let us know and we'll re-open the PR so that you can continue the contribution! |
This PR has been automatically marked as stale because it has not had recent activity from the author. It will be closed if no further activity occurs. If the PR was closed and you want it re-opened, let us know and we'll re-open the PR so that you can continue the contribution! |
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 for want to migrate this plugin here @kurtaking, this is a large undertaking.
I've tried to review this mostly from a house keeping perspective. There's a lot of deprecated code being used and some patterns that could be improved but it makes more send to get this big PR in and then follow up with PRs around specific items.
workspaces/announcements/env.sample
Outdated
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.
Do we need this file?
"scripts": { | ||
"dev": "concurrently \"yarn start\" \"yarn start-backend\"", | ||
"start": "yarn workspace @backstage-community/plugin-announcements start", | ||
"start-backend": "yarn workspace @backstage-community/plugin-announcements-backend start", | ||
"start:ci": "concurrently \"yarn start\" \"yarn start-backend:ci\"", | ||
"build": "backstage-cli repo build --all", | ||
"tsc": "tsc", | ||
"tsc:full": "tsc --skipLibCheck false --incremental false", | ||
"new": "backstage-cli new --scope internal", | ||
"clean": "backstage-cli repo clean", | ||
"test": "backstage-cli repo test", | ||
"test:all": "backstage-cli repo test --coverage", | ||
"lint": "backstage-cli repo lint --since origin/main", | ||
"lint:all": "backstage-cli repo lint", | ||
"version": "changeset version && yarn install && (git commit -am 'Update internal dependencies' || true)", | ||
"release": "changeset publish", | ||
"prepare": "husky install", | ||
"postinstall": "husky install || true" |
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.
Remove release
and prepare
, please.
@@ -0,0 +1,256 @@ | |||
# @backstage-community/plugin-announcements-backend |
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.
If we are going to keep the old changelogs then they should reflect the history. Looking at this you would think this plugin was alway part of the @backstage-community
scope.
@@ -0,0 +1,80 @@ | |||
# @backstage-community/plugin-announcements-react |
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.
These should properly reflect their history or be removed.
@@ -0,0 +1,342 @@ | |||
# @backstage-community/plugin-announcements |
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.
These should properly reflect their history or be removed.
yarn.lock
Outdated
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.
This file should not be delete, please revert this change 👍
@@ -0,0 +1,66 @@ | |||
# @backstage-community/plugin-search-backend-module-announcements |
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.
Last one, should reflect history or be removed
@@ -0,0 +1,35 @@ | |||
# Announcements plugin for Backstage | |||
|
|||
## Overview |
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.
Can we please add a note to this about the history of this plugin, looking for the ownership changes. If that is somewhere else please point that out, this is a big PR and I may have missed it 👍
Signed-off-by: Kurt King <[email protected]>
Co-authored-by: Andre Wanlin <[email protected]> Signed-off-by: Kurt King <[email protected]>
Signed-off-by: Kurt King <[email protected]>
…s into announcements
Closing as new PR #1637 was created |
Hey, I just made a Pull Request!
✔️ Checklist
Signed-off-by
line in the message. (more info)