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

Revert "Revert "fix(migrate): improve acl->authz migration, remove de… #338

Merged
merged 8 commits into from
Dec 7, 2022

Conversation

Avantol13
Copy link
Contributor

@Avantol13 Avantol13 commented Sep 28, 2022

…precated endpoints (#336)" (#337)"

COPY of #336 with addition of fixing broken integration test. Had previously force merged, reverted, now fixing

New Features

Breaking Changes

  • Ensure you are using Fence>=2021.10. Remove deprecated GA4GH access endpoints. these have existed (and been used) from the Fence microservice since last year. See cloud automation change here

Bug Fixes

Improvements

  • allow passing of fence config for authz mapping to the acl migration script

Dependency updates

Deployment changes

@github-actions
Copy link

The style in this PR agrees with black. ✔️

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

Comment on lines -216 to -217
raise EnvironmentError(
"program or project {} does not exist".format(acl_item)
Copy link
Contributor

Choose a reason for hiding this comment

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

should we keep this at least as a warning log?

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 warning would end up needing to be more generic b/c now it's not just that program/proj don't exist, but that there's no mapping either. I think the error log in a few lines sorta covers that? https://github.com/uc-cdis/indexd/pull/338/files/24af7553837da98ddf5a53259b91e4f87d23a936#diff-02f08e0cd101b79f6a25f6d6689008de374d7b6399a2a1291ee52faafc73aacdR247

Lemme know if you disagree

Copy link
Contributor Author

Choose a reason for hiding this comment

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

basically if nothing is found in any of the cases then there'll be an error log

Copy link
Contributor

Choose a reason for hiding this comment

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

ah ok, yeah that other log works

@paulineribeyre
Copy link
Contributor

i think we should add the changes to the PR description even if they have already been published in release notes

@Avantol13
Copy link
Contributor Author

Force merging even though a single test is failing. It is a known faulty test

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.

4 participants