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/usersync update role #843

Merged
merged 70 commits into from
Mar 3, 2021
Merged

Conversation

mattgarvin1
Copy link
Contributor

@mattgarvin1 mattgarvin1 commented Oct 13, 2020

Problem: previously the usersync job would fail to modify existing roles in Arborist -this is because the Arborist API didn't have a PUT /role/{roleID} endpoint

Solution: this PR solves this problem by having Fence hit the newly implemented PUT /role/{roleID} Arborist endpoint

Note: this is a non-destructive code change in that the new usersync logic is:

  • try to overwrite role by hitting the new Arborist endpoint
  • if role overwrite doesn't work (because an older version of Arborist is deployed which does not have the new PUT /role/{roleID} endpoint), go to the pre-existing logic flow, which results in creating new roles that don't already exist in Arborist but does NOT result in modifying existing roles

That is to say - ideally in a given environment Fence and Arborist would be updated to these latest versions simultaneously, BUT if for whatever reason that's not possible - it would not cause any errors to only update Fence with this new version without updating Arborist.

Similarly, it would not cause any errors to only update the Arborist deployment without updating Fence, because the only difference would be that now that Arborist deployment has a new API endpoint that simply doesn't get used anywhere.

Bug Fixes

  • during usersync - update existing roles in Arborist by hitting the new PUT /role/{roleID} endpoint

@github-actions
Copy link

github-actions bot commented Oct 13, 2020

The style in this PR agrees with black. ✔️

This formatting comment was generated automatically by a script in uc-cdis/wool.

@coveralls
Copy link

coveralls commented Oct 13, 2020

Pull Request Test Coverage Report for Build 10514

  • 1 of 9 (11.11%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.05%) to 69.535%

Changes Missing Coverage Covered Lines Changed/Added Lines %
fence/sync/sync_users.py 1 9 11.11%
Totals Coverage Status
Change from base Build 10509: -0.05%
Covered Lines: 5722
Relevant Lines: 8229

💛 - Coveralls

@lgtm-com
Copy link

lgtm-com bot commented Oct 13, 2020

This pull request introduces 1 alert when merging 4c25019 into 50ff568 - view on LGTM.com

new alerts:

  • 1 for Except block handles 'BaseException'

Avantol13
Avantol13 previously approved these changes Oct 15, 2020
@lgtm-com
Copy link

lgtm-com bot commented Oct 15, 2020

This pull request introduces 1 alert when merging f6bcb31 into ba0b1fc - view on LGTM.com

new alerts:

  • 1 for Except block handles 'BaseException'

fence/sync/sync_users.py Outdated Show resolved Hide resolved
@lgtm-com
Copy link

lgtm-com bot commented Oct 19, 2020

This pull request introduces 1 alert when merging 76f4461 into 9826912 - view on LGTM.com

new alerts:

  • 1 for Except block handles 'BaseException'

@mattgarvin1 mattgarvin1 dismissed Avantol13’s stale review December 8, 2020 16:53

needs re-review after dep update.

Copy link
Contributor

@paulineribeyre paulineribeyre left a comment

Choose a reason for hiding this comment

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

lgtm

fence/sync/sync_users.py Outdated Show resolved Hide resolved
@mysterious-progression mysterious-progression merged commit 3994a51 into master Mar 3, 2021
@mysterious-progression mysterious-progression deleted the fix/usersync-update-role branch March 3, 2021 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants