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

[NBS 1.0] deprecate legacy logger helpers #24804

Merged
merged 6 commits into from
May 21, 2024
Merged

[NBS 1.0] deprecate legacy logger helpers #24804

merged 6 commits into from
May 21, 2024

Conversation

camilaibs
Copy link
Contributor

@camilaibs camilaibs commented May 16, 2024 •
edited

Hey, I just made a Pull Request!

Refs: #24803

It is part of the effort to deprecate the legacy backend system and this pull request deprecates the logger helpers.

✔️ 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)

@camilaibs
refactor: start deprecating legacy service helpers
819754e 
Signed-off-by: Camila Belo <camilaibs@gmail.com>
@camilaibs
refactor: stop using getVoidLogger in tests
eb34b87 
Signed-off-by: Camila Belo <camilaibs@gmail.com>
@github-actions github-actions bot added area:catalog Related to the Catalog Project Area auth area:techdocs Related to the TechDocs Project Area area:kubernetes Related to the Kubernetes Project Area - not deploying Backstage with k8s. search Things related to Search area:scaffolder Everything and all things related to the scaffolder project area area:permission Related to the Permission Project Area area:discoverability Related to the Discoverability Project Area area:events Related to the Events Project Area labels May 16, 2024
@backstage-goalie
Copy link
Contributor

backstage-goalie bot commented May 16, 2024 •
edited

Changed Packages

Package Name Package Path Changeset Bump Current Version
@backstage/backend-app-api packages/backend-app-api patch v0.7.3
@backstage/backend-common packages/backend-common patch v0.22.0
@backstage/backend-tasks packages/backend-tasks patch v0.5.23
@backstage/backend-test-utils packages/backend-test-utils patch v0.3.8
@backstage/plugin-catalog-backend-module-bitbucket-cloud plugins/catalog-backend-module-bitbucket-cloud patch v0.2.5
@backstage/plugin-events-node plugins/events-node patch v0.3.4
@internal/plugin-todo-list-backend plugins/example-todo-list-backend none v1.0.27
@backstage/plugin-scaffolder-node-test-utils plugins/scaffolder-node-test-utils patch v0.1.4
@backstage/plugin-search-backend-module-elasticsearch plugins/search-backend-module-elasticsearch patch v1.4.1
@backstage/plugin-search-backend-module-pg plugins/search-backend-module-pg patch v0.5.27
@backstage/plugin-search-backend-node plugins/search-backend-node patch v1.2.22
@backstage/plugin-signals-backend plugins/signals-backend patch v0.1.4

camilaibs
camilaibs commented May 16, 2024
View reviewed changes
packages/backend-test-utils/src/next/services/mockServices.ts
/** @internal */
function createLoggerMock() {
return {
child: jest.fn().mockImplementation(createLoggerMock),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note

For some tests, this was failing because the child function returned undefined previously. There is a fragility with this solution, if the resetAllMocks is used, the implementation mock will be removed and the tests calling child will fail again. Any suggestions to prevent this from happening?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, if you use jest.fn(createLoggerMock) here instead, does that have an effect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It has no effect, if reset is called, child becomes just a noop function

camilaibs
camilaibs commented May 16, 2024
View reviewed changes
plugins/catalog-backend-module-aws/src/providers/AwsS3EntityProvider.test.ts
@@ -79,7 +79,7 @@ describe('AwsS3EntityProvider', () => {
});

afterEach(() => {
jest.resetAllMocks();
jest.clearAllMocks();
Copy link
Contributor Author

@camilaibs camilaibs May 16, 2024 •
edited

Choose a reason for hiding this comment

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

Note

As commented before, I'm replacing jest.resetAllMocks() with jest.clearAllMocks( ) to avoid resetting the child method mock implementation.

@camilaibs camilaibs mentioned this pull request May 16, 2024
Closed
9 tasks
@camilaibs camilaibs linked an issue May 16, 2024 that may be closed by this pull request
Deprecate and remove all legacy common functionality that has already been replaced in the new system #24803
Closed
9 tasks
@camilaibs
refactor: create a void logger service impl
a98f851 
Signed-off-by: Camila Belo <camilaibs@gmail.com>
@camilaibs
docs: add changeset files
6a576dc 
Signed-off-by: Camila Belo <camilaibs@gmail.com>
@camilaibs camilaibs force-pushed the camilaibs/nbs10-deprecate-legacy-system-commons branch from 89380c8 to 6a576dc Compare May 16, 2024 09:25
Rugvip
Rugvip requested changes May 17, 2024
View reviewed changes
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.

👌 🧹 ✨

.changeset/late-ants-impress.md Outdated Show resolved Hide resolved
packages/backend-app-api/src/logging/VoidLogger.ts Outdated Show resolved Hide resolved
awanlin
awanlin approved these changes May 17, 2024
View reviewed changes
Copy link
Collaborator

@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 for DevTools, thanks @camilaibs 🚀

@camilaibs camilaibs force-pushed the camilaibs/nbs10-deprecate-legacy-system-commons branch from 2c39e74 to d025975 Compare May 18, 2024 19:18
@camilaibs camilaibs requested a review from Rugvip May 18, 2024 19:21
@camilaibs
refactor: apply review suggestions
e49d0fd 
Signed-off-by: Camila Belo <camilaibs@gmail.com>
@camilaibs camilaibs force-pushed the camilaibs/nbs10-deprecate-legacy-system-commons branch from d025975 to e49d0fd Compare May 18, 2024 19:22
@camilaibs
Merge branch 'master' into camilaibs/nbs10-deprecate-legacy-system-co…
737f3ae 
…mmons

Signed-off-by: Camila Belo <camilaibs@gmail.com>
Rugvip
Rugvip approved these changes May 21, 2024
View reviewed changes
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.

:shipit: 🎉

@Rugvip Rugvip merged commit 41b942f into master May 21, 2024
37 checks passed
@Rugvip Rugvip deleted the camilaibs/nbs10-deprecate-legacy-system-commons branch May 21, 2024 14:36
@github-actions GitHub Actions
Copy link
Contributor

Thank you for contributing to Backstage! The changes in this pull request will be part of the 1.28.0 release, scheduled for Tue, 18 Jun 2024.

@camilaibs camilaibs mentioned this pull request May 21, 2024
Open
@github-actions GitHub Actions
Copy link
Contributor

Uffizzi Cluster pr-24804 was deleted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Rugvip Rugvip Rugvip approved these changes

@awanlin awanlin awanlin approved these changes

@pjungermann pjungermann Awaiting requested review from pjungermann pjungermann is a code owner

@backstage-service backstage-service Awaiting requested review from backstage-service backstage-service is a code owner

@benjdlambert benjdlambert Awaiting requested review from benjdlambert benjdlambert is a code owner automatically assigned from backstage/maintainers

@vinzscam vinzscam Awaiting requested review from vinzscam vinzscam is a code owner automatically assigned from backstage/reviewers

@alde alde Awaiting requested review from alde alde is a code owner automatically assigned from backstage/catalog-maintainers

Assignees
No one assigned
Labels
area:catalog Related to the Catalog Project Area area:discoverability Related to the Discoverability Project Area area:events Related to the Events Project Area area:kubernetes Related to the Kubernetes Project Area - not deploying Backstage with k8s. area:permission Related to the Permission Project Area area:scaffolder Everything and all things related to the scaffolder project area area:techdocs Related to the TechDocs Project Area auth search Things related to Search
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Deprecate and remove all legacy common functionality that has already been replaced in the new system
3 participants
@camilaibs @Rugvip @awanlin