search

casbin-net / casbin-aspnetcore

Casbin.NET integration middleware and sample code for ASP.NET Core
https://github.com/casbin/Casbin.NET
Apache License 2.0
64 stars 20 forks source link

[bug] the commit "fix: not use ref values to transform" failed the CI tests #59

Closed hsluoyz closed 1 year ago

hsluoyz commented 1 year ago

This commit: https://github.com/casbin-net/casbin-aspnetcore/commit/cf65b40e573da70e164dcc06e53285a7fd6d7620 has introduced bugs and caused the CI tests to fail: https://github.com/casbin-net/casbin-aspnetcore/actions/runs/4470605463/jobs/7854387801 . This commit has been reverted and backed-up at another branch: https://github.com/casbin-net/casbin-aspnetcore/tree/sagilio

@sagilio next time, please make a PR so we can track whether the code has broken any CI

casbin-bot commented 1 year ago

@sagilio @sociometry @AsakusaRinne

sagilio commented 1 year ago

Ok. I will fix it later. This commit will fix another BUG, we can not reset it.

hsluoyz commented 1 year ago

@sagilio why did you remove my fix for: https://github.com/casbin-net/casbin-aspnetcore/issues/58#issuecomment-1492857520 ? The fix commit: https://github.com/casbin-net/casbin-aspnetcore/commit/d0fa72b804822b2bcccbf2134a6768ac00a4f4cd is not in any branch now

sagilio commented 1 year ago

@sagilio why did you remove my fix for: #58 (comment) ? The fix commit: d0fa72b is not in any branch now

@hsluoyz

p, alice@example.com, BasicTest, Get

This policy is an example of the basic transformer, it is not wrong here. it needs the user to change the default transformer options to test it. I commented on the reason here (https://github.com/casbin-net/casbin-aspnetcore/issues/58#issuecomment-1493089944).

In fact, I have tried to enhance the experience for the user trying the example by setting the transformer at the attribute. But the change is in the reverted commit and there is a little logic conflict with your commit

So I recovered the backup branch and only cherry-pick the last commit to make sure the last version worked well.

hsluoyz commented 1 year ago

it needs the user to change the default transformer options to test it. I commented on the reason here (https://github.com/casbin-net/casbin-aspnetcore/issues/58#issuecomment-1493089944).

I don't think it makes sense if it has made a lot of people think it's a bug. It's not a good example code. If you want to show some special transformer functionality, then you create a "working" example especially for it. The example itself SHOULD be working out-of-box. Above all, the result we see is that the user experience has been impacted by the wrongly-designed example, long-time no response in the issue, and broken CI for several months. I don't think this is a good way to maintain the software responsibly.

sagilio commented 1 year ago

it needs the user to change the default transformer options to test it. I commented on the reason here (#58 (comment)).

I don't think it makes sense if it has made a lot of people think it's a bug. It's not a good example code. If you want to show some special transformer functionality, then you create a "working" example especially for it. The example itself SHOULD be working out-of-box. Above all, the result we see is that the user experience has been impacted by the wrongly-designed example, long-time no response in the issue, and broken CI for several months. I don't think this is a good way to maintain the software responsibly.

You are right, the example code needs to enhance and I'm also sorry for the lack of time to positively maintain it lately, I will improve it.