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

ShortURL Plugins #1222

Open
wants to merge 30 commits into
base: main
Choose a base branch
from
Open

ShortURL Plugins #1222

wants to merge 30 commits into from

Conversation

avila-m-6
Copy link

@avila-m-6 avila-m-6 commented Sep 14, 2024

Hey, I just made a Pull Request!

This is the set of ShortURL plugins, both frontend and backend sharing workspace.

This plugin will enable the creation of short urls within /shorturl and redirection through /go:id.

Features:

  1. Create a persistent short url
  2. Read/filter existing URLs
  3. Get redirected through backstage to a long url using HOST/go/:id
  4. Auto-copy new url on creation
  5. The DB stores only the random string and destination, making the URLs reusable across deployments
  6. Usage "metrics" (only counts) per link
  7. URL format validation on creation attempt

✔️ Checklist

  • A changeset describing the change and affected packages. (more info)
  • Added or updated documentation
  • Tests for new functionality and regression tests for bug fixes
  • Screenshots attached (for UI changes)
  • All your commits have a Signed-off-by line in the message. (more info)

image

Redirect page has a progress bar for loading, and an error panel for failures:

image

This is WIP. Next steps:

  1. Actually integrate the frontend with backend
  2. Add route to access the links through backstage.com/go/AAABBB
  3. Auto-copy created url into clipboard
  4. Add error handling
  5. Add tests
  6. Make the redirect page friendlier

This is related to #221 and #1220

Signed-off-by: Marcello Avila <[email protected]>
Signed-off-by: Marcello Avila <[email protected]>
Signed-off-by: Marcello Avila <[email protected]>
Signed-off-by: Marcello Avila <[email protected]>
Signed-off-by: Marcello Avila <[email protected]>
Signed-off-by: Marcello Avila <[email protected]>
Signed-off-by: Marcello Avila <[email protected]>
@avila-m-6 avila-m-6 marked this pull request as draft September 14, 2024 05:36
@backstage-goalie
Copy link
Contributor

backstage-goalie bot commented Sep 14, 2024

Unnecessary Changesets

The following package(s) are private and do not need a changeset:

  • @backstage-community/plugin-shorturl-backend
  • @backstage-community/plugin-shorturl

Changed Packages

Package Name Package Path Changeset Bump Current Version
@backstage-community/plugin-shorturl-backend workspaces/shorturl/plugins/shorturl-backend minor v0.1.0
@backstage-community/plugin-shorturl workspaces/shorturl/plugins/shorturl minor v0.1.0

@billabongrob
Copy link
Contributor

@avila-m-6 Do you think it would make sense to also support bringing your own short URL? I started a bit of work on this as well since it's an immediate need, but what I'm thinking is tying the route /shorturl to a path /go, then supporting the /go/:id
example. So if your network team is nice enough to do some aliasing for b.c to backstage.corporate.com then you could share URLs such as b.c/go/infosec or b.c/go/support - just spitballing.
Another concern I have is removing the auth requirements on /shorturl/:id but not /shorturl, so as to support a clean redirect. Just random musings when thinking about the functionality.

I really like the simplicity of the creation at the top and the thoughtfulness to automatically copy it into the clipboard!

@avila-m-6
Copy link
Author

I like the idea, but how about for a next iteration? So we get the ball rolling and some users validation.

And I would like you to describe better that auth removal intent

@billabongrob
Copy link
Contributor

totally fair @avila-m-6
The only reason I mentioned the auth removal was so that if Backstage were handling the redirect, users didn't have to auth into Backstage in order to be redirected, if that makes sense? Makes the redirect process a bit longer.

@avila-m-6
Copy link
Author

Ah yes, I like that. It makes sense as shorturl is no auth provider. Just a translator. It shouldn't ever have any sensitive data. I didn't add any authentication on the plugin though. That's on the backend integration step

* use discoverApi for api interaction
* use configApi for url listing
* validate url input format
* refresh list on url creation across components
* auto-copy to clipboard on creation
* implement alertApi for verbosity
* implement create and get apis
* update backend api for creation values

Signed-off-by: Marcello Avila <[email protected]>
@avila-m-6 avila-m-6 changed the title Shorturl frontend ShortURL Plugins Sep 18, 2024
@avila-m-6 avila-m-6 mentioned this pull request Sep 18, 2024
5 tasks
Signed-off-by: Marcello Avila <[email protected]>
Signed-off-by: Marcello Avila <[email protected]>
Signed-off-by: Marcello Avila <[email protected]>
@avila-m-6
Copy link
Author

Only frontend tests are missing. But logic should be good to go

@avila-m-6 avila-m-6 marked this pull request as ready for review September 18, 2024 16:42
Signed-off-by: Marcello Avila <[email protected]>
@avila-m-6
Copy link
Author

@BethGriggs, sorry to tag. I'm unclear on the next steps. Also I was instructed by the automation on #1220 that no changeset was required. But I saw a run that failed for a missing changeset?

https://github.com/backstage/community-plugins/actions/runs/10917413204/job/30371450366#step:6:8

@BethGriggs
Copy link
Collaborator

API report generation is failing in https://github.com/backstage/community-plugins/actions/runs/11020142480/job/30676982272?pr=1222

I do believe we should add changesets to trigger the release process.

Copy link
Contributor

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!

@github-actions github-actions bot added the stale label Oct 16, 2024
@avila-m-6
Copy link
Author

@BethGriggs, letting you know in case stale tag does not get removed automatically. I've added the changesets and api-reports :)

@github-actions github-actions bot removed the stale label Oct 17, 2024
@awanlin awanlin self-requested a review October 21, 2024 18:25
Copy link
Contributor

@awanlin awanlin left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @avila-m-6, left a few comments, nothing huge but should be looked at before we can merge this in.

Also, cna you please add yourself to the CODEOWNERS file: https://github.com/backstage/community-plugins/blob/main/.github/CODEOWNERS

workspaces/shorturl/app-config.yaml Outdated Show resolved Hide resolved
workspaces/shorturl/plugins/.changeset/lovely-games-mix.md Outdated Show resolved Hide resolved
workspaces/shorturl/plugins/shorturl-backend/README.md Outdated Show resolved Hide resolved
workspaces/shorturl/plugins/shorturl-backend/README.md Outdated Show resolved Hide resolved
"resolutions": {
"@types/react": "^18",
"@types/react-dom": "^18",
"@microsoft/api-extractor": "7.36.4"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this resolution, it's no longer needed. You'll also need to do a version bump to use the latest Backstage packages from 1.32.x. Finally, this change will rename the API reports following a new naming convention so make sure to re-generate them.

Copy link
Author

Choose a reason for hiding this comment

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

Done!

Copy link
Contributor

Choose a reason for hiding this comment

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

It's still here 🤔

workspaces/shorturl/plugins/shorturl/README.md Outdated Show resolved Hide resolved
workspaces/shorturl/plugins/shorturl/README.md Outdated Show resolved Hide resolved
@backstage-goalie
Copy link
Contributor

Thanks for the contribution!
All commits need to be DCO signed before they are reviewed. Please refer to the the DCO section in CONTRIBUTING.md or the DCO status for more info.

Signed-off-by: Marcello Avila <[email protected]>
Signed-off-by: Marcello Avila <[email protected]>
Signed-off-by: Marcello Avila <[email protected]>
Copy link
Contributor

@awanlin awanlin left a comment

Choose a reason for hiding this comment

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

Thanks for resolving the comments @avila-m-6, just some simple adjustments to the config.d.ts and getting the build green at which point we will be able to approve and merge this in.

"resolutions": {
"@types/react": "^18",
"@types/react-dom": "^18",
"@microsoft/api-extractor": "7.36.4"
Copy link
Contributor

Choose a reason for hiding this comment

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

It's still here 🤔

workspaces/shorturl/plugins/shorturl-backend/config.d.ts Outdated Show resolved Hide resolved
workspaces/shorturl/plugins/shorturl-backend/config.d.ts Outdated Show resolved Hide resolved
workspaces/shorturl/plugins/shorturl-backend/config.d.ts Outdated Show resolved Hide resolved
@avila-m-6
Copy link
Author

I took a good read at the api reports resources but I don't understand why they still came as api-report.md.api.md.

I did the v1.32 bump after removing the resolution.

I read through backstage/backstage#25671 and #886

Copy link
Contributor

@awanlin awanlin left a comment

Choose a reason for hiding this comment

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

Looks like there's still issues with the API Reports.

@@ -0,0 +1 @@
{ "version": "1.30.0" }
Copy link
Contributor

Choose a reason for hiding this comment

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

This tells me you are still on an older version which is probably why the API Reports are not coming up correctly

Copy link
Contributor

Choose a reason for hiding this comment

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

You should be able to run yarn backstage-cli versions:bump --release main from the root of your workspace: '/workspaces/shorturl. Then commit those changes. Once that's done then try building the API Reports again with yarn build:api-reports

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.

5 participants