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

#24889 - Feature: Fetch Gitlab users for self-hosted Gitlab based on group assignment #24936

Merged
merged 12 commits into from
Jun 25, 2024
Merged

#24889 - Feature: Fetch Gitlab users for self-hosted Gitlab based on group assignment #24936

merged 12 commits into from
Jun 25, 2024

Conversation

hghtwr
Copy link
Contributor

@hghtwr hghtwr commented May 27, 2024

Hey, I just made a Pull Request!

In reference to the problem described in #24889, this PR implements a new configuration key to limit the ingested users to the group defined in the provider configuration for self-hosted and SaaS.

I have tested this positively with a self-hosted Gitlab instance. Unforunately, I don't have access to a SaaS instance even though the change on SaaS side is only a minor one that shouldn't break anything.

It also slips a small change into the .gitignore to add .envrc files used with direnv.

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

@hghtwr hghtwr requested review from a team and backstage-service as code owners May 27, 2024 21:45
@github-actions github-actions bot added documentation Improvements or additions to documentation area:catalog Related to the Catalog Project Area labels May 27, 2024
@backstage-goalie
Copy link
Contributor

backstage-goalie bot commented May 27, 2024 •
edited
Loading

Changed Packages

Package Name Package Path Changeset Bump Current Version
example-backend packages/backend none v0.0.27
@backstage/plugin-catalog-backend-module-gitlab plugins/catalog-backend-module-gitlab patch v0.3.18

@hghtwr
Copy link
Contributor Author

hghtwr commented May 28, 2024

Im gonna clean this up in the course of the day...

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented May 28, 2024 •
edited
Loading

Uffizzi Ephemeral Environment - Virtual Cluster

Your cluster pr-24936 was successfully created. Learn more about Uffizzi virtual clusters
To connect to this cluster, follow these steps:

  1. Download and install the Uffizzi CLI from https://docs.uffizzi.com/install
  2. Login to Uffizzi, then select the backstage account and project:
uffizzi login

Select an account: 
  ‣ backstage
    jdoe

Select a project or create a new project: 
  ‣ backstage-6783521
  1. Update your kubeconfig: uffizzi cluster update-kubeconfig pr-24936 --kubeconfig=[PATH_TO_KUBECONFIG]
    After updating your kubeconfig, you can manage your cluster with kubectl, kustomize, helm, and other tools that use kubeconfig files: kubectl get namespace --kubeconfig [PATH_TO_KUBECONFIG]

Access the backstage endpoint at https://backstage-default-pr-24936-c5595.uclusters.app.uffizzi.com

@hghtwr hghtwr marked this pull request as draft May 28, 2024 12:46
@vinzscam
Copy link
Member

Nice! Feel free to remove it from draft whenever is ready so that we get pinged :D

@hghtwr
Copy link
Contributor Author

hghtwr commented Jun 2, 2024

@vinzscam I'm pretty lost with the last two failing checks as I don't have a good understanding about yarn workspaces.
I can reproduce the issue locally by removing the yarn.lock file but I got no idea where it is coming from? The PR doesnt change any dependencies. It doesn't happen using the original lockfile...

@vinzscam
Copy link
Member

vinzscam commented Jun 4, 2024

@vinzscam I'm pretty lost with the last two failing checks as I don't have a good understanding about yarn workspaces. I can reproduce the issue locally by removing the yarn.lock file but I got no idea where it is coming from? The PR doesnt change any dependencies. It doesn't happen using the original lockfile...

try rebasing master, probably something was wrong in the base branch

@hghtwr @SoJoh
Implement group-based user retrieval for Gitlab based on additional c…
115741c 
…onfiguration key 'restrictUsersToGroup'

Signed-off-by: Hghtwr <johannes.sonner@outlook.com>
@hghtwr @SoJoh
Create changeset for restrictUserToGroup configuration key
8db30ad 
Signed-off-by: Hghtwr <johannes.sonner@outlook.com>
@hghtwr @SoJoh
Add explaination for Gitlab's restrictUsersToGroup configuration key
59df048 
Signed-off-by: Hghtwr <johannes.sonner@outlook.com>
@hghtwr @SoJoh
Remove gitlab-org used during testing, remove unused types and options
5d4045b 
Signed-off-by: Hghtwr <johannes.sonner@outlook.com>
@hghtwr @SoJoh
Remove former implementation of listSaaSUsers()
70500dd 
Signed-off-by: Hghtwr <johannes.sonner@outlook.com>
@hghtwr @SoJoh
Satisfy Typecheck for user/groups by restructuring the if clause
8f34253 
Signed-off-by: Hghtwr <johannes.sonner@outlook.com>
@hghtwr @SoJoh
Update api report for backstage#24889
f79798b 
Signed-off-by: Hghtwr <johannes.sonner@outlook.com>
@hghtwr @SoJoh
Remove unused dependencies after testing
20de0f8 
Signed-off-by: Hghtwr <johannes.sonner@outlook.com>
@hghtwr @SoJoh
Fix reviewdog documentation issues
08b0740 
Signed-off-by: Hghtwr <johannes.sonner@outlook.com>
@hghtwr hghtwr marked this pull request as ready for review June 5, 2024 09:22
@hghtwr
Copy link
Contributor Author

hghtwr commented Jun 5, 2024

@vinzscam Thanky you, that worked. The DCO job seems to be stuck before even starting, so I hope that won't be a problem.

benjdlambert
benjdlambert requested changes Jun 18, 2024
View reviewed changes
Copy link
Member

@benjdlambert benjdlambert left a comment

Choose a reason for hiding this comment

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

Thanks for this, just want to cover the additional functionality with some tests if thats possible 🙏

plugins/catalog-backend-module-gitlab/src/providers/GitlabOrgDiscoveryEntityProvider.ts Show resolved Hide resolved
@hghtwr
Extend tests for subgroups
2537c3f 
Signed-off-by: Hghtwr <johannes.sonner@outlook.com>
@hghtwr
add self-hosted test suite, fix format
c67d887 
Signed-off-by: Hghtwr <johannes.sonner@outlook.com>
@hghtwr
revert EntityProvider signature
206698c 
Signed-off-by: Hghtwr <johannes.sonner@outlook.com>
@hghtwr hghtwr requested a review from benjdlambert June 21, 2024 09:43
benjdlambert
benjdlambert reviewed Jun 24, 2024
View reviewed changes
.yarnrc.yml Outdated Show resolved Hide resolved
benjdlambert
benjdlambert reviewed Jun 25, 2024
View reviewed changes
plugins/catalog-backend-module-gitlab/src/providers/GitlabOrgDiscoveryEntityProvider.test.ts
@@ -260,15 +260,16 @@ describe('GitlabOrgDiscoveryEntityProvider - refresh', () => {
const taskDef = schedule.getTasks()[0];

await (taskDef.fn as () => Promise<void>)();
const userEntities = mock.expected_full_org_scan_entities.filter(
const entities = mock.expected_full_org_scan_entities.filter(
Copy link
Member

Choose a reason for hiding this comment

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

Does this logic change the default behaviour which we have today or am I misreading this test change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The full org scan returns users as well as groups (and their subgroups).
Previously, there was no testing with subgroups. I implemented this into the test data. So the comparison of returned users and the returned entities doesn't make a lot of sense anymore.

I can make the test more precise by filtering the returned entities to users, compare this to the expected users and add another line of test that filters the returned entities to groups and compares them to the expected number of groups. That would keep the original test logic in place but still would be compatible to the test data.

benjdlambert
benjdlambert approved these changes Jun 25, 2024
View reviewed changes
Copy link
Member

@benjdlambert benjdlambert left a comment

Choose a reason for hiding this comment

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

OK - let's go with this 🎉 thanks!

@benjdlambert benjdlambert merged commit 9de9d08 into backstage:master Jun 25, 2024
32 checks passed
@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.29.0 release, scheduled for Tue, 16 Jul 2024.

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

@benjdlambert benjdlambert benjdlambert approved these changes

@backstage-service backstage-service Awaiting requested review from backstage-service

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

Assignees
No one assigned
Labels
area:catalog Related to the Catalog Project Area documentation Improvements or additions to documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@hghtwr @vinzscam @benjdlambert