-
Notifications
You must be signed in to change notification settings - Fork 0
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
bug internal: Added VO to list_scopes and test_rse_expression_parser #39, Fix #38, #25 #40
bug internal: Added VO to list_scopes and test_rse_expression_parser #39, Fix #38, #25 #40
Conversation
…om/stfc/rucio into feature-2635-multiVO_functionality_PA Merging bugfix to account_limits
@@ -63,7 +64,7 @@ def get(self): | |||
:status 406: Not Acceptable | |||
:returns: :class:`String` | |||
""" | |||
return dumps(list_scopes()) | |||
return dumps(list_scopes(filter={}, vo=request.environ.get('vo'))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't think there's any need for filter={}
to be here (pedantically trying to avoid clutter)
@@ -58,7 +59,7 @@ def GET(self): | |||
HTTP Error: | |||
406 Not Acceptable | |||
""" | |||
return dumps(list_scopes()) | |||
return dumps(list_scopes(filter={}, vo=ctx.env.get('vo'))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also didn't think it needed it, and this bit wasn't in my first (2) commit(s), but I inconsistently got test failures (you can see these in the first two Travis suites, it seemed to happen randomly).
Adding filter={}
stopped these errors (I think the underlying cause was it converts filter from a string to an InternalScope
, but in some cases it tried to re-use the last value of filter it had). This was the easiest way I could find to fix this, but I can look back into it if you think it's worth fixing in a different way? Other places in the REST layer (like account) can pass through {}
, but admittedly only when they don't get given any other parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, weird. That's definitely suspicious and could be worth looking into (maybe make a low-priority issue?), but if this fixes it then I'm happy for it to stay in - just wanted to minimise clutter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created the issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comments for discussion
@@ -84,68 +86,84 @@ def __init__(self): | |||
@raises(InvalidRSEExpression) | |||
def test_unconnected_operator(self): | |||
""" RSE_EXPRESSION_PARSER (CORE) Test invalid rse expression: unconnected operator""" | |||
rse_expression_parser.parse_expression("TEST_RSE1|") | |||
rse_expression = add_vo_to_rse_expression("TEST_RSE1|", **self.vo) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests seem designed to test one specific aspect of the RSE expression each. Adding the VO to them all suddenly means there are multiple possible failing points (e.g. we go from 'only |
can cause a failure' to 'failure could be any of |&()
).
I'm wondering if there's a way to filter by VO specifically in these tests instead, e.g. by using [... for t_rse in <result> if t_rse['vo']==self.vo['vo'] ]
on the output. Maybe not necessary in every case, but in e.g. test_simple_rse_reference
it seems like we're no longer really testing a 'simple expression' when we tack the VO on the front.
In some cases e.g. test_unconnected_operator
I don't think the VO is necessary at all as it's just a syntax test.
What do you think on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you mean, but given that we run a full suite in both single and multi VO mode, we'd still test the simple syntax in the single vo suite (as we don't modify the vo expression if vo==def), and then test that syntax with VO additions in mutli VO mode only.
At the time I didn't really consider each test seperately, I just modified them all, so if you do think any would be better as they were I don't mind reverting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I forgot that in the single VO mode the expression is unchanged. The changes do clearly affect the clarity of the code, though (if it caught me out it could catch others), so I'm going to mull this one over a little bit more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After continued mulling, I think this is ok. The clarity of it still bothers me though, so I'm open to further discussion later if anyone has better ideas.
Initial misunderstanding and subsequent discussion (see comments)
Added basic tests for interference between 2 VOs for adding/listing accounts, DIDs, RSEs and scopes.
This lead to fixing bugs with
list_scopes
not taking VO, andtest_rse_expression_parser
not adding VO to its RSE expressions.