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

feat: Add GetDomainsForUser API #161

Merged
merged 1 commit into from
May 22, 2021
Merged

feat: Add GetDomainsForUser API #161

merged 1 commit into from
May 22, 2021

Conversation

sagilio
Copy link
Member

@sagilio sagilio commented May 21, 2021

Fixes: #147

Can not update DynamicExpresso.Core to v2.5.0: dynamicexpresso/DynamicExpresso#144

@sagilio sagilio added enhancement Enhancement the exist feature new feature New feature will be provided or request labels May 21, 2021
@sagilio sagilio self-assigned this May 21, 2021
@sagilio sagilio requested a review from xcaptain May 21, 2021 18:06
@hsluoyz
Copy link
Member

hsluoyz commented May 22, 2021

@thoraj plz review.

Signed-off-by: Sagilio <Sagilio@outlook.com>
@thoraj
Copy link

thoraj commented May 22, 2021

@thoraj plz review.

I will be happy to review. Is there a nuget feed where I can pick up the build?

@hsluoyz
Copy link
Member

hsluoyz commented May 22, 2021

@sagilio is there a nightly nuget feed for this PR, which can be used for testing?

@sagilio sagilio marked this pull request as draft May 22, 2021 16:31
@sagilio
Copy link
Member Author

sagilio commented May 22, 2021

@hsluoyz Secrets are not passed to workflows that are triggered by a pull request from a fork. Pushing myget packages needs the MYGET_TOKEN.

@sagilio
Copy link
Member Author

sagilio commented May 22, 2021

@thoraj plz review.

I will be happy to review. Is there a nuget feed where I can pick up the build?

I have changed the CI. We can merge first, It will release on MyGet feed.
You can use this version to test

@sagilio sagilio marked this pull request as ready for review May 22, 2021 17:07
@sagilio sagilio merged commit a5122e8 into casbin:master May 22, 2021
sagilio added a commit to sagilio/Casbin.NET that referenced this pull request May 22, 2021
feat: Add GetDomainsForUser API
sagilio added a commit that referenced this pull request May 22, 2021
Signed-off-by: Sagilio <Sagilio@outlook.com>
@sagilio
Copy link
Member Author

sagilio commented May 22, 2021

@hsluoyz Secrets are not passed to workflows that are triggered by a pull request from a fork. Pushing myget packages needs the MYGET_TOKEN.

@hsluoyz So, I try to split the build and release workflow to build and release (use event trigger here) and they still use the semantic release.
I think It will make the release more flexible. e.g.: Sometimes we need more than one PR to release a new version.

@hsluoyz
Copy link
Member

hsluoyz commented May 23, 2021

@sagilio I didn't see a new release coming after this PR merge. We still should do this:

Let's say the latest version is v1.2.2. When we have new PR:

  1. Make nightly release to MyGet for each new PR update, using v1.2.2-1, v1.2.2-2, etc.
  2. Make semantic-release to NuGet after PR merge using v1.2.3.

@sagilio
Copy link
Member Author

sagilio commented May 23, 2021

@hsluoyz We can not release to MyGet from fork repo.
image

Now, the release policy like this. it only added one more confirmation after merge:

  1. Make nightly release to MyGet after PR merge (We also can view the semantic release dry-run result now).
  2. Make semantic release to NuGet after manually trigger the release workflow.

I think this is a better way for this repo. In fact, it more like the build pipeline and release pipeline in Azure DevOps.
I have triggered a release just now and the v1.8.0 version is released, https://github.com/casbin/Casbin.NET/actions/runs/867798888.

@hsluoyz
Copy link
Member

hsluoyz commented May 23, 2021

@sagilio how to trigger semantic release for me?

@sagilio
Copy link
Member Author

sagilio commented May 23, 2021

@thoraj
Copy link

thoraj commented May 23, 2021

I noticed there was a new package v1.8.0 on nuget so I ran some test on that. I got the expected results so LGTM.

I only tested providing a user parameter like this.

domains = enforcer.GetDomainsForUser("@user1");

I did not provide anything for the roleType, as I'm not sure what to expect for these cases.

@sagilio
Copy link
Member Author

sagilio commented May 23, 2021

@thoraj It will use the default role type.

public const string DefaultRoleType = "g";

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement the exist feature new feature New feature will be provided or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Getting a list of domains where a user has permissions
3 participants