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

Add endpoint_manager pause_rule methods and unit tests #203

Merged
merged 3 commits into from
Apr 24, 2017

Conversation

aaschaer
Copy link
Contributor

Resolves #197

Once this is merged I'll go back and test things that were blocked on pausing.

@aaschaer aaschaer added this to the management-console milestone Apr 20, 2017
@sirosen
Copy link
Member

sirosen commented Apr 20, 2017

The TransferClient tests are getting awfully long -- we might want to try to segment those tests into a few files/modules just for ease of management/perusal.

This looks good, assuming that tests pass.

@aaschaer
Copy link
Contributor Author

Ah, I forgot to merge params in pause_rule_list.

I'm slightly disappointed in myself for this, but also proud of myself for writing tests that caught my mistake.

@aaschaer aaschaer force-pushed the endpoint_manager_pause_rules branch from 795d342 to 0dcecc2 Compare April 20, 2017 21:00
@aaschaer
Copy link
Contributor Author

2 of the failures were a race condition I assumed would never happen in an unrelated test and a connection error.

The third however is still violating my assertion that only one pause rule will be created per shared endpoint in the pause rule tests.
Either the filter parameter still isn't working, or something has gone terribly wrong. I'll see if I can figure out which.

@aaschaer
Copy link
Contributor Author

Ah, I think I was somehow still looking at the old failures (maybe caused by my rebase?) since now there is only one failure.

I'll test some more locally, but I think the problem is fixed now?

@aaschaer
Copy link
Contributor Author

Upon closer inspection, it seems the filter_host_endpoint param is either being ignored by the API, or not behaving as expected (according to the docs I should have been getting back an empty list).

I removed the named parameter, but if it is behaving properly for actual host endpoint managers it can still be passed through **params.

I'll add this to the list of small bugs / documentation inconsistencies with the APIs I've run into testing endpoint_manager calls. Where should I make issues to track those?

@sirosen
Copy link
Member

sirosen commented Apr 24, 2017

I'll add this to the list of small bugs / documentation inconsistencies with the APIs I've run into testing endpoint_manager calls. Where should I make issues to track those?

I'd say that you should file these directly against Transfer, and that way we can figure out if they need to be fixed there (underlying bug) or just in the docs (misdocumented feature / outdated docs). If we go straight to the docs, it may get fixed to reflect the actual state of the system even if we don't really want it that way.

Please do so in the language of "GET against /foo/bar", rather than the language of "SDK call TransferClient.foo(bar)".

@sirosen sirosen merged commit 5c2288f into globus:master Apr 24, 2017
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