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.

Sign up for GitHub

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

Jump to bottom

feat(tech-insights): allow usage of API client from backend plugins #630

Merged
merged 13 commits into from
Aug 30, 2024
Merged

feat(tech-insights): allow usage of API client from backend plugins #630

merged 13 commits into from
Aug 30, 2024

Conversation

secustor
Copy link
Member

@secustor secustor commented Jul 15, 2024 •
edited
Loading

Hey, I just made a Pull Request!

Moves the TechInsightsClient class to @backstage-community/plugin-tech-insights-common package under a new /client export.

This is to enable usage of the client for other backend plugins.
To enable this, the client constructor accepts now IdentityApi or AuthService as parameter.

✔️ Checklist

  • A changeset describing the change and affected packages. (more info)
  • 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)

@secustor
feat(tech-insights): move client into separate package and allow usin…
4f19c43 
…g backend auth

Signed-off-by: secustor <sebastian@poxhofer.at>
@secustor secustor requested review from Xantier and a team as code owners July 15, 2024 15:51
@secustor secustor requested a review from nickboldt July 15, 2024 15:51
awanlin
awanlin requested changes Jul 15, 2024
View reviewed changes
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.

Hi @secustor, thanks for this, just wanted to share a naming comment so you can handle that early.

workspaces/tech-insights/plugins/tech-insights-client/package.json Outdated Show resolved Hide resolved
@backstage-goalie
Copy link
Contributor

backstage-goalie bot commented Jul 15, 2024 •
edited
Loading

Changed Packages

Package Name Package Path Changeset Bump Current Version
@backstage-community/plugin-tech-insights-common workspaces/tech-insights/plugins/tech-insights-common patch v0.2.16
@backstage-community/plugin-tech-insights workspaces/tech-insights/plugins/tech-insights patch v0.3.33

@awanlin
Copy link
Contributor

awanlin commented Jul 15, 2024

For the current CI error you are going to need to update the Backstage CLI version. Essentially the package is missing the plugin metadata which will cause the process to publish to NPM to fail.

@secustor
feat(tech-insights): move client into common package under /client ex…
d6e85d8 
…port

Signed-off-by: secustor <sebastian@poxhofer.at>
@secustor secustor changed the title feat(tech-insights): move client into separate package feat(tech-insights): allow usage of API client from backend plugins Jul 16, 2024
@secustor
feat(tech-insights): run yarn fix
a279212 
Signed-off-by: secustor <sebastian@poxhofer.at>
@secustor
remove backend from changeset
aa8b84d 
Signed-off-by: secustor <sebastian@poxhofer.at>
@secustor
update api report
e4111f5 
Signed-off-by: secustor <sebastian@poxhofer.at>
@secustor
add client api report
f67e8c0 
Signed-off-by: secustor <sebastian@poxhofer.at>
@secustor
add missing documentation and try to solve api report problem
b207a0a 
Signed-off-by: secustor <sebastian@poxhofer.at>
@secustor

This comment was marked as outdated.

Sign in to view
@secustor
pin version
bdab5c0 
Signed-off-by: secustor <sebastian@poxhofer.at>
@secustor secustor mentioned this pull request Jul 19, 2024
Merged
4 tasks
@secustor
Merge branch 'refs/heads/main' into feat/move-client-in-separate-package
9f44ac4 
Signed-off-by: secustor <sebastian@poxhofer.at>

# Conflicts:
#	workspaces/tech-insights/plugins/tech-insights-common/package.json
#	workspaces/tech-insights/yarn.lock
@secustor
dedupe lock file
f1d019b 
Signed-off-by: secustor <sebastian@poxhofer.at>
@secustor secustor requested a review from awanlin July 22, 2024 10:34
awanlin
awanlin approved these changes Jul 24, 2024
View reviewed changes
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 good, would like a review from @Xantier as well 👍

@awanlin
Copy link
Contributor

awanlin commented Aug 23, 2024

Hi @Xantier, could really use your eyes on this one, please?

@secustor, looks like a conflict crept in, sorry this is taking so long.

Xantier
Xantier reviewed Aug 23, 2024
View reviewed changes
Copy link
Contributor

@Xantier Xantier left a comment

Choose a reason for hiding this comment

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

Looks to be a straightforward lift, right? Looks fine to me, just update the texts and typos and this should be good to go

@secustor
Copy link
Member Author

It's not a complete shift.

There is now a function to abstract token fetching. https://github.com/backstage/community-plugins/pull/630/files#diff-18721d7dfb9f9c8fec43c477a4c28706fe62585cf382069c8c122ca12c68ead9R151-R164

secustor and others added 3 commits August 28, 2024 19:23
@secustor @Xantier
Apply suggestions from code review
ae24634 
Co-authored-by: Jussi Hallila <jussi@hallila.com>
Signed-off-by: Sebastian Poxhofer <secustor@users.noreply.github.com>
@secustor
Merge branch 'main' into feat/move-client-in-separate-package
ca5d134 
Signed-off-by: secustor <sebastian@poxhofer.at>

# Conflicts:
#	workspaces/tech-insights/plugins/tech-insights/api-report.md
#	workspaces/tech-insights/plugins/tech-insights/src/api/TechInsightsClient.ts
#	workspaces/tech-insights/yarn.lock
@secustor
Merge remote-tracking branch 'origin/feat/move-client-in-separate-pac…
bf47d4c 
…kage' into feat/move-client-in-separate-package
@secustor secustor requested a review from Xantier August 28, 2024 18:33
@secustor
Copy link
Member Author

@nickboldt Sry, for the ping but you are a CodeOwner and this PR is already open for a while.
Would you be able to review it?

Thanks!

@BethGriggs BethGriggs merged commit a84eb44 into backstage:main Aug 30, 2024
12 checks passed
@secustor secustor deleted the feat/move-client-in-separate-package branch August 30, 2024 13:22
BethGriggs pushed a commit that referenced this pull request Aug 30, 2024
@secustor @Xantier @BethGriggs
feat(tech-insights): allow usage of API client from backend plugins (#…
fd70333 
…630)

* feat(tech-insights): move client into separate package and allow using backend auth

Signed-off-by: secustor <sebastian@poxhofer.at>

* feat(tech-insights): move client into common package under /client export

Signed-off-by: secustor <sebastian@poxhofer.at>

* feat(tech-insights): run yarn fix

Signed-off-by: secustor <sebastian@poxhofer.at>

* remove backend from changeset

Signed-off-by: secustor <sebastian@poxhofer.at>

* update api report

Signed-off-by: secustor <sebastian@poxhofer.at>

* add client api report

Signed-off-by: secustor <sebastian@poxhofer.at>

* add missing documentation and try to solve api report problem

Signed-off-by: secustor <sebastian@poxhofer.at>

* pin version

Signed-off-by: secustor <sebastian@poxhofer.at>

* dedupe lock file

Signed-off-by: secustor <sebastian@poxhofer.at>

* Apply suggestions from code review

Co-authored-by: Jussi Hallila <jussi@hallila.com>
Signed-off-by: Sebastian Poxhofer <secustor@users.noreply.github.com>

---------

Signed-off-by: secustor <sebastian@poxhofer.at>
Signed-off-by: Sebastian Poxhofer <secustor@users.noreply.github.com>
Co-authored-by: Jussi Hallila <jussi@hallila.com>
karthikjeeyar pushed a commit to karthikjeeyar/community-plugins that referenced this pull request Oct 28, 2024
@secustor @Xantier @karthikjeeyar
feat(tech-insights): allow usage of API client from backend plugins (b…
daa09ab 
…ackstage#630)

* feat(tech-insights): move client into separate package and allow using backend auth

Signed-off-by: secustor <sebastian@poxhofer.at>

* feat(tech-insights): move client into common package under /client export

Signed-off-by: secustor <sebastian@poxhofer.at>

* feat(tech-insights): run yarn fix

Signed-off-by: secustor <sebastian@poxhofer.at>

* remove backend from changeset

Signed-off-by: secustor <sebastian@poxhofer.at>

* update api report

Signed-off-by: secustor <sebastian@poxhofer.at>

* add client api report

Signed-off-by: secustor <sebastian@poxhofer.at>

* add missing documentation and try to solve api report problem

Signed-off-by: secustor <sebastian@poxhofer.at>

* pin version

Signed-off-by: secustor <sebastian@poxhofer.at>

* dedupe lock file

Signed-off-by: secustor <sebastian@poxhofer.at>

* Apply suggestions from code review

Co-authored-by: Jussi Hallila <jussi@hallila.com>
Signed-off-by: Sebastian Poxhofer <secustor@users.noreply.github.com>

---------

Signed-off-by: secustor <sebastian@poxhofer.at>
Signed-off-by: Sebastian Poxhofer <secustor@users.noreply.github.com>
Co-authored-by: Jussi Hallila <jussi@hallila.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aramissennyeydd aramissennyeydd aramissennyeydd left review comments

@awanlin awanlin awanlin approved these changes

@BethGriggs BethGriggs BethGriggs approved these changes

@Xantier Xantier Xantier approved these changes

@nickboldt nickboldt Awaiting requested review from nickboldt nickboldt is a code owner automatically assigned from backstage/community-plugins-maintainers

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@secustor @awanlin @Xantier @BethGriggs @aramissennyeydd