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

continued continued move of core services to backend-defaults #24849

Closed
wants to merge 4 commits into from

Conversation

freben
Copy link
Member

@freben freben commented May 21, 2024

This is a continuation of #24724, moving over some implementations from backend-common (which is scheduled for removal) to backend-defaults.

  • urlReader

Made this into a PR of its own since it turned out to be pretty large. Note that it's on top of #24848, which should be merged first. But therefore, piggybacking on its changesets.

This PR does NOT rename the types in backend-plugin-api, e.g. SearchOptions -> UrlReaderSearchOptions, since that's an additional pretty large change that can come by itself so things are easier to review in isolation.

Signed-off-by: Fredrik Adelöw <freben@gmail.com>
Signed-off-by: Fredrik Adelöw <freben@gmail.com>
Signed-off-by: Fredrik Adelöw <freben@gmail.com>
Signed-off-by: Fredrik Adelöw <freben@gmail.com>
@freben freben requested review from backstage-service and a team as code owners May 21, 2024 12:52
@freben freben requested review from Rugvip and vinzscam and removed request for a team May 21, 2024 12:52
Copy link
Member

@Rugvip Rugvip left a comment

Choose a reason for hiding this comment

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

Sweet! 👍

Wonder if we should move + relative import though? We'd have the duplicate implementations in place for at least a couple of releases and bit worried that's gonna add too much friction.

@freben freben changed the title continued continued move of core services to backend-common continued continued move of core services to backend-defaults May 21, 2024
@freben
Copy link
Member Author

freben commented May 21, 2024

@Rugvip Ooh maybe. Hm which one should have the implementation though? That would suggest that the impl should stay in backend-common, since backend-defaults already has a dependency on it. Don't wanna introduce circular dependencies

@freben freben force-pushed the freben/defaults-continued branch 2 times, most recently from a561d8e to 201ad33 Compare May 21, 2024 13:45
Copy link
Contributor

@camilaibs camilaibs left a comment

Choose a reason for hiding this comment

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

Thanks @freben 🙌🏻

@freben freben force-pushed the freben/defaults-continued branch 7 times, most recently from e5fce40 to 02103be Compare May 22, 2024 13:29
@freben
Copy link
Member Author

freben commented May 22, 2024

I think I'll start over on this one now that #24848 settled on a slightly different structure

@freben freben closed this May 22, 2024
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.

3 participants