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

Permission update #46

Merged
merged 20 commits into from
Nov 30, 2023
Merged

Permission update #46

merged 20 commits into from
Nov 30, 2023

Conversation

Nazim-crim
Copy link
Contributor

@Nazim-crim Nazim-crim commented Nov 13, 2023

  • Add optional key field and regex to be used in the sync_permissions section found in the config.
    This allows to sync permissions using a field other than resource_full_name when creating the name:type
    from the segment ex.: /field1::type1/field2::type2. Adds support to use resource_display_name.
  • The regex is used to extract the desired information from the nametype_path. It should be used to do an
    exact match. This new search overrides the default way of matching each segment with the nametype_path.
    In the case where a regex is found in the target segment, the data will be formed using the same resource_type
    for every match in the same segment. Similary, as using - name: "**" in the config to match multiple segment,
    it is possible to use a regex to match multiple resources in the same segment with regex: '(?<=:).*\/?(?=\/)'

@Nazim-crim Nazim-crim self-assigned this Nov 13, 2023
@github-actions github-actions bot added api Issue related to API functionalities. doc Updates to documentation. labels Nov 13, 2023
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Thanks for submitting a PR. Make sure you have looked at the contribution guidelines. Also, look for quick check/tests operations that you can run locally for early verification of errors. Travis will be happier if it doesn't need to run too many times with problematic code.

Copy link
Collaborator

@fmigneault fmigneault left a comment

Choose a reason for hiding this comment

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

Make sure that all tests and checks pass in the CI.

Also, add at least 1 test that demonstrates and validates the new feature, so we can get a better understanding of what is expected, and to make sure that future updates don't break it either.

An example in https://github.com/Ouranosinc/cowbird/blob/master/config/config.example.yml (with a few comments for each step/new field) would be needed to indicate and explain this new capability.

cowbird/config.py Outdated Show resolved Hide resolved
cowbird/permissions_synchronizer.py Outdated Show resolved Hide resolved
cowbird/permissions_synchronizer.py Outdated Show resolved Hide resolved
cowbird/permissions_synchronizer.py Outdated Show resolved Hide resolved
cowbird/permissions_synchronizer.py Outdated Show resolved Hide resolved
cowbird/permissions_synchronizer.py Outdated Show resolved Hide resolved
cowbird/permissions_synchronizer.py Outdated Show resolved Hide resolved
cowbird/permissions_synchronizer.py Show resolved Hide resolved
cowbird/permissions_synchronizer.py Show resolved Hide resolved
Copy link
Contributor

@ChaamC ChaamC left a comment

Choose a reason for hiding this comment

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

Along with the changes required by @fmigneault (tests and config example update), the doc found in configuration.rst might be needing an update too.

cowbird/api/webhooks/views.py Outdated Show resolved Hide resolved
cowbird/permissions_synchronizer.py Outdated Show resolved Hide resolved
cowbird/permissions_synchronizer.py Outdated Show resolved Hide resolved
@github-actions github-actions bot added the ci Updates to the CI pipelines and related utilities. label Nov 15, 2023
Copy link

codecov bot commented Nov 15, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (4a5e77d) 87.57% compared to head (9d25376) 87.76%.
Report is 12 commits behind head on master.

Files Patch % Lines
cowbird/permissions_synchronizer.py 97.67% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master      #46      +/-   ##
==========================================
+ Coverage   87.57%   87.76%   +0.19%     
==========================================
  Files          40       40              
  Lines        3211     3245      +34     
  Branches      471      480       +9     
==========================================
+ Hits         2812     2848      +36     
+ Misses        278      276       -2     
  Partials      121      121              

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

@github-actions github-actions bot added the tests Modifications to tests or problems related to test suite. label Nov 24, 2023
config/config.example.yml Outdated Show resolved Hide resolved
config/config.example.yml Outdated Show resolved Hide resolved
cowbird/config.py Show resolved Hide resolved
tests/test_permissions_synchronizer.py Show resolved Hide resolved
tests/test_permissions_synchronizer.py Outdated Show resolved Hide resolved
tests/test_permissions_synchronizer.py Outdated Show resolved Hide resolved
cowbird/handlers/impl/magpie.py Outdated Show resolved Hide resolved
cowbird/config.py Show resolved Hide resolved
CHANGES.rst Outdated Show resolved Hide resolved
CHANGES.rst Outdated Show resolved Hide resolved
CHANGES.rst Outdated Show resolved Hide resolved
CHANGES.rst Outdated Show resolved Hide resolved
docs/configuration.rst Outdated Show resolved Hide resolved
docs/configuration.rst Outdated Show resolved Hide resolved
docs/configuration.rst Outdated Show resolved Hide resolved
config/config.example.yml Outdated Show resolved Hide resolved
docs/configuration.rst Outdated Show resolved Hide resolved
@Nazim-crim Nazim-crim requested a review from ChaamC November 29, 2023 19:11
fmigneault
fmigneault previously approved these changes Nov 29, 2023
docs/configuration.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@ChaamC ChaamC left a comment

Choose a reason for hiding this comment

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

Good for me, nice job!

@Nazim-crim Nazim-crim merged commit b64c86d into master Nov 30, 2023
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Issue related to API functionalities. ci Updates to the CI pipelines and related utilities. doc Updates to documentation. tests Modifications to tests or problems related to test suite.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants