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

plugin(analytics-provider-segment): migration of analytics-provider-segment plugin to backstage/community-plugins workspace #1896

Closed
wants to merge 27 commits into from

Conversation

Fortune-Ndlovu
Copy link
Contributor

@Fortune-Ndlovu Fortune-Ndlovu commented Nov 8, 2024

Hey, I just made a Pull Request!

The analytics-provider-segment plugin from the janus-idp/backstage-plugins repository was migrated to the community plugins. The migration was performed by following the manual migration steps outlined in the Community Plugins CONTRIBUTING guide

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

@backstage-goalie
Copy link
Contributor

backstage-goalie bot commented Nov 8, 2024

Missing Changesets

The following package(s) are changed by this PR but do not have a changeset:

  • @backstage-community/plugin-analytics-module-ga
  • @backstage-community/plugin-analytics-module-ga4
  • @backstage-community/plugin-analytics-module-newrelic-browser

See CONTRIBUTING.md for more information about how to add changesets.

Changed Packages

Package Name Package Path Changeset Bump Current Version
@backstage-community/plugin-analytics-module-ga workspaces/analytics/plugins/analytics-module-ga none v0.2.9
@backstage-community/plugin-analytics-module-ga4 workspaces/analytics/plugins/analytics-module-ga4 none v0.2.11
@backstage-community/plugin-analytics-module-newrelic-browser workspaces/analytics/plugins/analytics-module-newrelic-browser none v0.1.9
@backstage-community/plugin-analytics-provider-segment workspaces/analytics/plugins/analytics-provider-segment patch v1.10.1

@Fortune-Ndlovu
Copy link
Contributor Author

cc: @04kash / @nickboldt

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.

@04kash
Copy link
Member

04kash commented Nov 8, 2024

Should this live in the analytics workspace? https://github.com/backstage/community-plugins/tree/main/workspaces/analytics

Hmm yeah I think so too, WDYT @Fortune-Ndlovu ?

@Fortune-Ndlovu
Copy link
Contributor Author

Agreed, moving...

@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.

@Fortune-Ndlovu
Copy link
Contributor Author

Thanks, @04kash, @awanlin!

@04kash
Copy link
Member

04kash commented Nov 8, 2024

Looks good! Just need to add a changeset

Fortune-Ndlovu and others added 14 commits November 8, 2024 18:34
Signed-off-by: Fortune-Ndlovu <fndlovu@redhat.com>
Signed-off-by: Fortune-Ndlovu <fndlovu@redhat.com>
Signed-off-by: Fortune-Ndlovu <fndlovu@redhat.com>
Signed-off-by: Fortune-Ndlovu <fndlovu@redhat.com>
Signed-off-by: Fortune-Ndlovu <fndlovu@redhat.com>
Signed-off-by: Fortune-Ndlovu <fndlovu@redhat.com>
Signed-off-by: Fortune-Ndlovu <fndlovu@redhat.com>
Signed-off-by: Fortune-Ndlovu <fndlovu@redhat.com>
Signed-off-by: Fortune-Ndlovu <fndlovu@redhat.com>
Signed-off-by: Fortune-Ndlovu <fndlovu@redhat.com>
Signed-off-by: Fortune-Ndlovu <fndlovu@redhat.com>
…i-reports

Signed-off-by: Fortune-Ndlovu <fndlovu@redhat.com>
Signed-off-by: MT Lewis <mtlewis@users.noreply.github.com>
Signed-off-by: Fortune-Ndlovu <fndlovu@redhat.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Signed-off-by: Fortune-Ndlovu <fndlovu@redhat.com>
Fortune-Ndlovu and others added 10 commits November 8, 2024 18:34
Signed-off-by: Fortune-Ndlovu <fndlovu@redhat.com>
Signed-off-by: Fortune-Ndlovu <fndlovu@redhat.com>
Signed-off-by: MT Lewis <mtlewis@users.noreply.github.com>
Signed-off-by: Fortune-Ndlovu <fndlovu@redhat.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Signed-off-by: Fortune-Ndlovu <fndlovu@redhat.com>
Signed-off-by: Fortune-Ndlovu <fndlovu@redhat.com>
Signed-off-by: MT Lewis <mtlewis@users.noreply.github.com>
Signed-off-by: Fortune-Ndlovu <fndlovu@redhat.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Signed-off-by: Fortune-Ndlovu <fndlovu@redhat.com>
Signed-off-by: Fortune-Ndlovu <fndlovu@redhat.com>
…rsion

Signed-off-by: Fortune-Ndlovu <fndlovu@redhat.com>
@nickboldt
Copy link
Collaborator

Needs an update to CODEOWNERS. See example in https://github.com/backstage/community-plugins/pull/1912/files

Signed-off-by: Fortune-Ndlovu <ndlovufortune97@gmail.com>
Signed-off-by: Fortune-Ndlovu <ndlovufortune97@gmail.com>
@nickboldt nickboldt requested a review from 04kash November 13, 2024 16:19
@04kash
Copy link
Member

04kash commented Nov 13, 2024

I see some unrelated commits as a part of this PR eg: d89bfc2, not sure how they got here

Copy link
Member

@04kash 04kash left a comment

Choose a reason for hiding this comment

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

I'm good with merging after this comment is addressed: #1896 (comment)

Comment on lines +1 to +8
{
"extends": ["//"],
"tasks": {
"tsc": {
"outputs": ["../../dist-types/plugins/analytics-provider-segment/**"]
}
}
}

Choose a reason for hiding this comment

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

We don't use turbo here, you can just remove that config.

Comment on lines +39 to +42
"@backstage/theme": "^0.6.0",
"@material-ui/core": "^4.9.13",
"@material-ui/icons": "^4.11.3",
"@material-ui/lab": "4.0.0-alpha.61",

Choose a reason for hiding this comment

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

It would surprise me if we need this dependency. But there was also in the old code base. We can remove them also in a follow-up PR!

Comment on lines +21 to +32
importOrder: [
'^react(.*)$',
'',
'^@backstage/(.*)$',
'',
'<THIRD_PARTY_MODULES>',
'',
'^@backstage-community/(.*)$',
'',
'<BUILTIN_MODULES>',
'',
'^[.]',

Choose a reason for hiding this comment

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

I don't know that plugin, this would be great, but it looks like it doesn't work as expected or do we need this for the other modules as well?

I think we should avoid extra prettier rules when possible.

Wdyt @awanlin @04kash ?

Comment on lines +21 to +24
import { createDevApp } from '@backstage/dev-utils';
import React from 'react';
import { GoogleAnalytics } from '../src';
import { Playground } from './Playground';

Choose a reason for hiding this comment

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

I saw that in different PRs, can we stay with the old order? I know, its an automated change, but this diff looks odd to me.

I didn't checked yet which config is responsible. Ping me if I should take a deeper look why some workspaces or plugins do this change and others not.

Wdyt @awanlin @04kash ?

@@ -11,7 +11,7 @@ yarn.lock @backstage/community-plugins
/workspaces/3scale @backstage/community-plugins-maintainers @04kash @AndrienkoAleksandr
/workspaces/acr @backstage/community-plugins-maintainers @christoph-jerolimov @invincibleJai
/workspaces/adr @backstage/community-plugins-maintainers @kuangp
/workspaces/analytics @backstage/community-plugins-maintainers @jmezach
/workspaces/analytics @backstage/community-plugins-maintainers @jmezach @schultzp2020 @christoph-jerolimov

Choose a reason for hiding this comment

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

Thanks for adding @schultzp2020 and me, but are we hi-jacking another workspace here?

@jmezach, is it fine for you if we just take care of the extra modules here, or are you fine if we also approve code changes to your modules?

Or, @awanlin @BethGriggs @04kash, should we consider adding additional lines here just for the module(s) we added? It's this PR for Google Analytics and #1906 for Matomo.

I just want to be a kindful and transparent citizen of this repo. ☮️ ✌️

Copy link
Contributor

Choose a reason for hiding this comment

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

I've added this as a topic for the next Community SIG, but my thinking is to have paths to each plugin in the CODEOWNERS file. @jmezach contributed the New Relic analytics plugin, the GA ones are from Spotify from what I can remember.

@Fortune-Ndlovu Fortune-Ndlovu closed this by deleting the head repository Nov 14, 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.

7 participants