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: register custom config model #189

Merged
merged 3 commits into from
Dec 4, 2023
Merged

Conversation

alohamora
Copy link
Contributor

@alohamora alohamora commented Dec 2, 2023

Description

This PR fixes the bug where foca did not register a custom config model which is passed in the input config if the api specs were not provided.
In addition, the validator for model path also converts the relative path passed in the config.yaml to the absolute path because casbin is not able to resolve relative path directly.

Fixes #181

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Copy link

codecov bot commented Dec 2, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (80f0136) 100.00% compared to head (9693596) 100.00%.

Additional details and impacted files
@@            Coverage Diff            @@
##               dev      #189   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           20        20           
  Lines          978       984    +6     
=========================================
+ Hits           978       984    +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@alohamora
Copy link
Contributor Author

@uniqueg I couldn't figure out on how to add tests for this specific change as it is just reading the model field from the config and converting it to an absolute path. I have tested this by manually passing model from cloud-registry and it is working fine.
Let me know how to handle testing for this

@uniqueg uniqueg changed the title fix: fix custom config usage behaviour fix: register custom config model Dec 3, 2023
Copy link
Member

@uniqueg uniqueg left a comment

Choose a reason for hiding this comment

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

This is a very elegant fix, thanks a lot

Copy link
Member

@uniqueg uniqueg left a comment

Choose a reason for hiding this comment

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

I have just seen the comment about the tests. I guess you could simply test it by instantiating the model with None, with a relative path, with an absolute path and with an invalid/non-existing path.

If you can add those tests and they pass, you can just go ahead and merge it. I don't think there will be a need for another review.

If you still have trouble writing the tests, just text in the #foca channel on Slack and I'll help you :)

@alohamora alohamora requested a review from uniqueg December 3, 2023 18:19
@alohamora
Copy link
Contributor Author

Hey @uniqueg can you please review the PR again, I was not confident if I was adding tests the correct way :)
Also, I did a small change of normalizing the relative paths which use .. notation by using Pathlib.resolve() method instead of manually constructing absolute path.

@alohamora alohamora merged commit 2c89ba8 into dev Dec 4, 2023
14 checks passed
@alohamora alohamora deleted the bug/access-control-model-fix branch December 4, 2023 08:36
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.

bug: unable to provide custom model config for access control
2 participants