Skip to content

Conversation

@ZohebShaikh
Copy link
Collaborator

No description provided.

@ZohebShaikh ZohebShaikh changed the title ci: add policy to ignore admin lint error ci: add policy to ignore admin lint error and update rust Nov 19, 2025
@ZohebShaikh ZohebShaikh changed the title ci: add policy to ignore admin lint error and update rust fix: Update is_admin function and update rust Nov 19, 2025
Copy link
Collaborator

@tpoliaw tpoliaw left a comment

Choose a reason for hiding this comment

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

Both parts look alright but why are they in the same PR?

Why is this implementation of is_admin better than the previous approach?

@ZohebShaikh
Copy link
Collaborator Author

The issue that regal was trying to point out was :

If you are storing the result of a decsion and then later using the stored decision then you might have stale decisions...

What might happen is lets say alice is currently a super_admin then all the other policy are using this output variable to make a decision whether alice is a super_admin then it will be get the old stored result rather than checking the if alice is a super_admin currently

Thats why changing into a function will always give you the correct decision

@ZohebShaikh ZohebShaikh changed the title fix: Update is_admin function and update rust fix: update is_admin function Dec 9, 2025
@ZohebShaikh ZohebShaikh changed the title fix: update is_admin function fix: change is_admin from array to function Dec 9, 2025
@ZohebShaikh ZohebShaikh requested a review from tpoliaw December 9, 2025 10:06
@ZohebShaikh
Copy link
Collaborator Author

@tpoliaw Have seperated them out into 2 prs here is the PR to update rust
#296

@tpoliaw tpoliaw dismissed their stale review December 9, 2025 10:23

PR split into two

@tpoliaw tpoliaw merged commit ff4deba into DiamondLightSource:main Dec 9, 2025
19 checks passed
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.

2 participants