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

Refactor of Pollable for modularization effort #118

Closed
wants to merge 2 commits into from

Conversation

tgross
Copy link
Contributor

@tgross tgross commented Mar 25, 2016

Making PollAction a method of Pollable will allow us to avoid a circular import when we split out packages from the main containerbuddy package.

@justenwalker this follows from the design I was describing in #83 (comment), which I think will make building the metrics and tasks packages easier and make our life easier when we split up the fat containerbuddy package into smaller bits. (Note that this only required changing one test, which relied on binding variables from the enclosing scope in poll, so I'm reasonably confident this is the minimal intervention we can make.)

cc @misterbisson

Making PollAction a method of Pollable will allow us to avoid a circular
import when we split out packages from the main containerbuddy package.
@tgross tgross mentioned this pull request Mar 25, 2016
@tgross
Copy link
Contributor Author

tgross commented Mar 28, 2016

I've also moved some of the parsing functions into their own package so that I can share them with packages that implement Pollable but want to validate their inputs (which I suspect all our Pollable-implementing packages will want to do).

Packages that include Pollables are going to want to validation their own
configuration, so by moving the parsing and IPs functions into a utils package we
can do so without any circular references.
@tgross
Copy link
Contributor Author

tgross commented Mar 29, 2016

This is safe to merge and release as a point release, as we've added new public APIs to our package (not that anyone is using the containerbuddy or utils package except us) but haven't eliminated anything and it's feature-identical. Did you have any thoughts on the design here, @justenwalker ?

@justenwalker
Copy link
Contributor

@tgross I haven't been able to pull this branch and play with it yet, however extracting out the utils package seems like a good idea. Also the PollAction seems like an appropriate abstraction - so from that standpoint is looks good.

I'm not sure how I'll be using it in the scheduled tasks, so I'd like to see if I can rebase on it and adapt what I have.

@tgross
Copy link
Contributor Author

tgross commented Mar 30, 2016

I'm not sure how I'll be using it in the scheduled tasks, so I'd like to see if I can rebase on it and adapt what I have.

Sounds good. Let's hold off merging this until we've both had a chance to try it out in the features we're working on.

@tgross tgross mentioned this pull request Mar 31, 2016
@tgross
Copy link
Contributor Author

tgross commented Mar 31, 2016

Alternately, if we decide we're happy with #123 that'll merge this as well.

@tgross
Copy link
Contributor Author

tgross commented Apr 8, 2016

I'm going to close this PR because it's covered by #123

@tgross tgross closed this Apr 8, 2016
@tgross tgross deleted the pollable_refactor branch April 4, 2017 15:46
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