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

fix(rbac): move admin creation to its own file #2349

Merged
merged 9 commits into from
Oct 21, 2024

Conversation

PatAKnight
Copy link
Member

@PatAKnight PatAKnight commented Oct 11, 2024

Description

Moves the admin creation to its own file to simplify the code base a little bit. This will also help in the future if we ever decide to allow for the reloading of admins from within the app-config.

Also included is a number of other updates.

  • Moves the test fixtures directory up a level
  • Includes a new mock-utils.ts file that holds all of the different mocks that we use in all of our test files
    • This is so we can further refactor our tests in the future to remove the redundant mocks throughout
  • Renames those redundant mocks to make the future update easier
  • Includes a new test-utils.ts file that holds some of the common test util functions that are used in our test files
    • This is so we can further refactor our tests in the future to remove the redundant function declarations throughout

The future refactor should help with some of the duplication issues that we are seeing within SonarCloud

@PatAKnight
Copy link
Member Author

The sonarcloud analysis is failing because of the amount of duplication that is in this PR. My plan is to clean this up in a separate PR.

Copy link

changeset-bot bot commented Oct 18, 2024

🦋 Changeset detected

Latest commit: dcb9437

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@janus-idp/backstage-plugin-rbac-backend Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@PatAKnight PatAKnight marked this pull request as ready for review October 18, 2024 15:56
@PatAKnight PatAKnight requested review from AndrienkoAleksandr and a team as code owners October 18, 2024 15:56
@PatAKnight PatAKnight requested a review from a team as a code owner October 18, 2024 16:02
Copy link
Collaborator

@AndrienkoAleksandr AndrienkoAleksandr left a comment

Choose a reason for hiding this comment

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

I really like this pull request and would like to merge it as soon as possible. However, I wonder if the admin-creation.ts file might be better placed in a separate package, perhaps something like 'admin-role' or similar, as it doesn’t seem directly related to file permissions. What are your thoughts on this?

@PatAKnight
Copy link
Member Author

@AndrienkoAleksandr I am fine with that. I only picked the file-permissions directory as technically the app-config is a file and we are using it to set permissions. But moving it also makes sense because the admin creation file is doing something completely different compared to the file watcher.

Copy link

sonarcloud bot commented Oct 21, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
25.5% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

@AndrienkoAleksandr
Copy link
Collaborator

/override sonarcloud

Copy link

openshift-ci bot commented Oct 21, 2024

@AndrienkoAleksandr: /override requires failed status contexts, check run or a prowjob name to operate on.
The following unknown contexts/checkruns were given:

  • sonarcloud

Only the following failed contexts/checkruns were expected:

  • Build with Node.js 20
  • Conventional Commits
  • Run Playwright Tests
  • SonarCloud Code Analysis
  • Test with Node.js 20
  • tide

If you are trying to override a checkrun that has a space in it, you must put a double quote on the context.

In response to this:

/override sonarcloud

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@AndrienkoAleksandr
Copy link
Collaborator

/override "SonarCloud Code Analysis"

Copy link

openshift-ci bot commented Oct 21, 2024

@AndrienkoAleksandr: Overrode contexts on behalf of AndrienkoAleksandr: SonarCloud Code Analysis

In response to this:

/override "SonarCloud Code Analysis"

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@AndrienkoAleksandr
Copy link
Collaborator

/lgtm

@openshift-merge-bot openshift-merge-bot bot merged commit a5abb98 into janus-idp:main Oct 21, 2024
9 of 10 checks passed
04kash pushed a commit to 04kash/backstage-plugins that referenced this pull request Oct 23, 2024
* fix(rbac): move admin creation to its own file

* fix(rbac): move test fixtures directory up a level

* fix(rbac): add mock utils fixture

* fix(rbac): rename mock services to be in line with fixtures mock

* fix(rbac): update import path

* fix(rbac): rename test file

* fix(rbac): add admin creation tests

* fix(rbac): add missing changeset

* fix(rbac): move admin creation to the admin-permissions directory
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants