8000 Introduce HttpProvider by jupe · Pull Request #15 · jupe/py-lockable · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Introduce HttpProvider #15

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
Merged
merged 15 commits into from
Aug 16, 2021
Merged

Introduce HttpProvider #15

merged 15 commits into from
Aug 16, 2021

Conversation

jupe
Copy link
Owner
@jupe jupe commented Aug 7, 2021 •
edited by juhhov
Loading

Changes:

  • extract resources provider from Lockable
    • Provider (abstract)
    • ProviderList (when resources is given as plain list of objects)
    • ProviderFile (when resources are given as file)
    • ProviderHttp (when resources are given as http url)
  • support http resources provider

Internal changes:

  • Lockable._resource_list reload list from file/http and return list of resource objects
  • Constructor (re)load resources

TODO:

  • Update readme

@jupe jupe requested a review from juhhov August 10, 2021 10:28
juhhov
juhhov reviewed Aug 10, 2021
View reviewed changes
juhhov
juhhov reviewed Aug 10, 2021
View reviewed changes
juhhov
juhhov reviewed Aug 10, 2021
View reviewed changes
@jupe jupe requested a review from juhhov August 11, 2021 17:15
@juhhov
Copy link
Collaborator
juhhov commented Aug 12, 2021

Ready for review?

@jupe jupe marked this pull request as ready for review August 12, 2021 07:32
@jupe
Copy link
Owner Author
jupe commented Aug 12, 2021

there is coverage decrease but Im not sure where it comes since it doesn’t happen in my machine… have to figure out still

juhhov
juhhov reviewed Aug 13, 2021
View reviewed changes
@juhhov
Copy link
Collaborator
juhhov commented Aug 16, 2021

Smoke tests of HTTP provider are positive. Noticed one possible issue though. Is it expected that lockable --resources http://localhost:3000/resources --requirements '{"online": true}' --hostname nuc01.maas ls leads two 4 separate get requests to server?

juhhov
juhhov reviewed Aug 16, 2021
View reviewed changes
juhhov
juhhov reviewed Aug 16, 2021
View reviewed changes
juhhov
juhhov requested changes Aug 16, 2021
View reviewed changes
Copy link
Collaborator
@juhhov juhhov left a comment

Choose a reason for hiding this comment

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

4 http calls per allocation seems too much.

juhhov
juhhov approved these changes Aug 16, 2021
View reviewed changes
Copy link
Collaborator
@juhhov juhhov left a comment

Choose a reason for hiding this comment

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

Please update README.

@jupe jupe merged commit c5b5d34 into master Aug 16, 2021
@jupe jupe deleted the resources_provider branch August 16, 2021 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@juhhov juhhov juhhov approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jupe @juhhov
0