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

catalog-backend-module-github-org: fix default namespace for groups #24700

Merged
merged 10 commits into from May 13, 2024

Conversation

vinzscam
Copy link
Member

@vinzscam vinzscam commented May 8, 2024

Hey, I just made a Pull Request!

As we are suggesting adopters to use the new catalog-backend-module-github-org when migrating to the new backend system, this PR fixes an issue where the catalog-backend-module-github-org would create groups using the org as namespace instead of the default namespace.

Now the module uses the default namespace for groups, in case a single org has been configured.

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

@vinzscam vinzscam requested review from a team as code owners May 8, 2024 18:30
@github-actions github-actions bot added the area:catalog Related to the Catalog Project Area label May 8, 2024
@backstage-goalie
Copy link
Contributor

Changed Packages

Package Name Package Path Changeset Bump Current Version
@backstage/plugin-catalog-backend-module-github-org plugins/catalog-backend-module-github-org patch v0.1.13-next.2
@backstage/plugin-catalog-backend-module-github plugins/catalog-backend-module-github patch v0.6.1-next.2

…Provider

Signed-off-by: Vincenzo Scamporlino <vincenzos@spotify.com>
Signed-off-by: Vincenzo Scamporlino <vincenzos@spotify.com>
Signed-off-by: Vincenzo Scamporlino <vincenzos@spotify.com>
Signed-off-by: Vincenzo Scamporlino <vincenzos@spotify.com>
@vinzscam vinzscam force-pushed the github-multi-org-fix-namespace branch from f73e6c9 to 11c9f1f Compare May 8, 2024 18:33
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.

Thanks a ton for this improvement @vinzscam 🚀

Signed-off-by: Vincenzo Scamporlino <vincenzos@spotify.com>
Signed-off-by: Vincenzo Scamporlino <vincenzos@spotify.com>
@github-actions github-actions bot added the documentation Improvements or additions to documentation label May 10, 2024
Signed-off-by: Vincenzo Scamporlino <vincenzos@spotify.com>
@vinzscam vinzscam force-pushed the github-multi-org-fix-namespace branch from 6520dc4 to 4cd526e Compare May 10, 2024 11:45
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.

Nice! 👍

Just a few nits

githubOrg:
- id: production
githubUrl: 'https://github.com',
orgs: ['org-a', 'org-b'],
Copy link
Member

Choose a reason for hiding this comment

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

Ah so it wasn't possible to configure this by org after all, that explains the code. Doesn't need to be done now but prolly makes sense to add support for Array<string | { name: string, groupNamespace?: string, userNamespace?: string}> in the future

*
* If set to true, groups with the same name across different orgs will be considered the same group.
*/
defaultNamespace?: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

Thinking that a good name for this could be alwaysUseDefaultNamespace?

entities: [],
})
.catch(error => {
console.error('Failed to clean up entities', error);
Copy link
Member

Choose a reason for hiding this comment

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

Grab the logger for this one :>

Copy link
Member Author

Choose a reason for hiding this comment

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

🤦

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.

Docs additions look good, thanks for adding that 🚀

Signed-off-by: Vincenzo Scamporlino <vincenzos@spotify.com>
Signed-off-by: Vincenzo Scamporlino <vincenzos@spotify.com>
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.

Bit awkward with the option, but that'll be cleaned up eventually as we move off the old system.

Good enough I'd say, and let's :shipit: 👍

Signed-off-by: Vincenzo Scamporlino <vincenzos@spotify.com>
@vinzscam vinzscam enabled auto-merge May 13, 2024 20:00
@vinzscam vinzscam merged commit 95ee6d6 into master May 13, 2024
30 checks passed
@vinzscam vinzscam deleted the github-multi-org-fix-namespace branch May 13, 2024 20:10
Copy link
Contributor

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

Copy link
Contributor

Uffizzi Cluster pr-24700 was deleted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:catalog Related to the Catalog Project Area documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants