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

Assign default roles from Teams to User #2947

Merged
merged 1 commit into from Feb 24, 2022

Conversation

mithmatt
Copy link
Collaborator

@mithmatt mithmatt commented Feb 23, 2022

Describe your changes :

See #2883

Type of change :

  • Bug fix
  • Improvement
  • New feature
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation

Frontend Preview (Screenshots) :

For frontend related change, please link screenshots of your changes preview! Optional for backend related changes.

Checklist:

  • I have read the CONTRIBUTING document.
  • I have performed a self-review of my own.
  • I have tagged my reviewers below.
  • I have commented on my code, particularly in hard-to-understand areas.
  • My changes generate no new warnings.
  • I have added tests that prove my fix is effective or that my feature works.
  • All new and existing tests passed.

Reviewers

Backend: @sureshms @harshach

@mithmatt mithmatt marked this pull request as draft February 24, 2022 06:37
@mithmatt mithmatt marked this pull request as ready for review February 24, 2022 06:50
@mithmatt
Copy link
Collaborator Author

pytest failure due to connection issue

Run sudo apt-get install -y libsasl2-dev unixodbc-dev python3-venv
Reading package lists...
Building dependency tree...
Reading state information...
python3-venv is already the newest version (3.[8](https://github.com/open-metadata/OpenMetadata/runs/5315142730?check_suite_focus=true#step:5:8).2-0ubuntu2).
unixodbc-dev is already the newest version (2.3.7).
The following NEW packages will be installed:
  libsasl2-dev
0 upgraded, 1 newly installed, 0 to remove and 33 not upgraded.
Need to get 225 kB of archives.
After this operation, 8[9](https://github.com/open-metadata/OpenMetadata/runs/5315142730?check_suite_focus=true#step:5:9)4 kB of additional disk space will be used.
Err:1 http://azure.archive.ubuntu.com/ubuntu focal/main amd64 libsasl2-dev amd64 2.1.27+dfsg-2
  Could not connect to azure.archive.ubuntu.com:80 (52.[14](https://github.com/open-metadata/OpenMetadata/runs/5315142730?check_suite_focus=true#step:5:14)7.219.192), connection timed out
E: Failed to fetch http://azure.archive.ubuntu.com/ubuntu/pool/main/c/cyrus-sasl2/libsasl2-dev_2.1.27+dfsg-2_amd64.deb  Could not connect to azure.archive.ubuntu.com:80 (52.147.2[19](https://github.com/open-metadata/OpenMetadata/runs/5315142730?check_suite_focus=true#step:5:19).192), connection timed out
E: Unable to fetch some archives, maybe run apt-get update or try with --fix-missing?

@vivekratnavel
Copy link
Contributor

Hi @mithmatt,

Thanks for the PR. Can you please take care of this test failure?

Errors: 
2022-02-24T07:06:04.7753691Z [ERROR]   UserResourceTest.userDefaultRoleAssignment:126->EntityResourceTest.patchEntityAndCheck:1480->EntityResourceTest.patchEntityAndCheck:1505->EntityResourceTest.patchEntity:1321 » HttpResponse 

@github-actions
Copy link

The Java checkstyle failed.

Please run mvn googleformatter:format@reformat-sources in the root of your repository and commit the changes to this PR.
You can also use pre-commit to automate the Java code formatting.

You can install the pre-commit hooks with make install_test precommit_install.

@mithmatt mithmatt force-pushed the user_team_role branch 2 times, most recently from b145216 to 1beb3ce Compare February 24, 2022 08:06
@@ -78,25 +79,19 @@ void defaultRole(TestInfo test) throws IOException {

List<User> users = new UserResourceTest().listEntities(Map.of("fields", "roles"), ADMIN_AUTH_HEADERS).getData();
for (User user : users) {
boolean defaultRoleSet = false, prevDefaultRoleExists = false;

for (EntityReference role : user.getRoles()) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Simplify the code - make it easy to understand

@@ -100,27 +101,90 @@ public void post_entity_as_non_admin_401(TestInfo test) {

@Order(Integer.MAX_VALUE) // Run this test last to avoid side effects of default role creation to fail other tests.
@Test
void post_userWithDefaultRole(TestInfo test) throws IOException {
void userDefaultRoleAssignment(TestInfo test) throws IOException {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Making this a comprehensive behavioral test

if (rolesRef == null || rolesRef.isEmpty()) {
rolesRef = new ArrayList<>();
// Add default roles from the teams that the user belongs to.
List<EntityReference> teamsRef = Optional.ofNullable(user.getTeams()).orElse(Collections.emptyList());
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the new logic being added

@mithmatt mithmatt force-pushed the user_team_role branch 2 times, most recently from 5f4a308 to 750666b Compare February 24, 2022 08:19
@sonarcloud
Copy link

sonarcloud bot commented Feb 24, 2022

[open-metadata-ingestion] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@mithmatt mithmatt merged commit 8e99867 into open-metadata:main Feb 24, 2022
@mithmatt mithmatt deleted the user_team_role branch February 24, 2022 08:47
@sonarcloud
Copy link

sonarcloud bot commented Feb 24, 2022

[catalog] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

90.5% 90.5% Coverage
0.0% 0.0% Duplication

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.

None yet

3 participants