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.

Sign up for GitHub

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

Jump to bottom

[#222] Set ADMIN_KW when user applying metadata is a rodsadmin (main) #225

Merged
merged 2 commits into from
Oct 13, 2023
Merged

[#222] Set ADMIN_KW when user applying metadata is a rodsadmin (main) #225

merged 2 commits into from
Oct 13, 2023

Conversation

korydraughn
Copy link
Collaborator

No description provided.

alanking
alanking reviewed Oct 11, 2023
View reviewed changes
Copy link
Contributor

@alanking alanking left a comment

Choose a reason for hiding this comment

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

Seems good to me. Should we add a test?

@trel
Copy link
Member

trel commented Oct 11, 2023

agreed. but we should see it behave in a test. this repo needs to be handsfree/trustworthy once it works.

@korydraughn
Copy link
Collaborator Author

I was able to reproduce the CAT_NO_ACCESS_PERMISSION error using the test on main. Adding in the fix resolves that issue resulting in a passing test.

@trel
Copy link
Member

trel commented Oct 13, 2023

love it.

trel
trel approved these changes Oct 13, 2023
View reviewed changes
Copy link
Member

@trel trel left a comment

Choose a reason for hiding this comment

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

remove those two lines if you want - i see they're for debugging.

otherwise, looks good.

@korydraughn korydraughn mentioned this pull request Oct 13, 2023
Merged
alanking
alanking approved these changes Oct 13, 2023
View reviewed changes
Copy link
Contributor

@alanking alanking left a comment

Choose a reason for hiding this comment

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

It does indeed test the behavior. Looks good

@alanking alanking merged commit fceeec0 into irods:main Oct 13, 2023
1 check passed
@korydraughn korydraughn deleted the 222.m branch October 13, 2023 18:32
@trel
Copy link
Member

trel commented Oct 13, 2023

that was close.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trel trel trel approved these changes

@alanking alanking alanking approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@korydraughn @trel @alanking