Remove dependencies from procfs and sysfs onto xfs, nfs, bcache, and iostats #136
Conversation
pgier
force-pushed
the
xfs-nfs-refactor
branch
from
010a303 to
44e2f3a
Compare
pgier
changed the title
|
From a quick look, it looks ok to me. |
|
I'm not familiar with this code. Wouldn't @grobie be the most natural reviewer? |
|
@grobie isn't maintaining this much anymore. We're currently working on finding a new owner. |
|
Understood. Currently, I cannot volunteer as a new maintainer for lack of time. For the time being, I would still prefer if @grobie could have a look here. I guess his lack of time is as bad as mine, but he would need less time to review this as he already knows the code. |
pgier
changed the title
|
@grobie Can you just let us know if you will be reviewing this? I know you wanted to propose doing exactly this IIRC, so naturally you'd be the best person to review this. |
|
I'll give it a look tonight or tomorrow night. Thanks for mentioning.
…On Sun, Mar 17, 2019, 10:19 discordianfish ***@***.***> wrote:
@grobie <https://github.com/grobie> Can you just let us know if you will
be reviewing this? I know you wanted to propose doing exactly this IIRC, so
naturally you'd be the best person to review this.
If you don't have time, no worries. I'll just do my best to review :)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#136 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAANaPpTbKWGv2OmbUyprkGOpXynpSQoks5vXgi6gaJpZM4bFrqx>
.
|
There was a problem hiding this comment.
Great seeing the coupling between the procfs package and other packages being broken up! Changing the interface to require the procfs mountpoint as first argument appears to be a reasonable tradeoff between package isolation and simplicity vs. code repetition. The idea of having a filesystem structure didn't scale well with the introduction of additional packages.
I left a few minor comments, but don't have bigger concerns. Do you plan to adjust the functions in procfs accordingly so that all functions have a similar signature?
Remove the dependencies from procfs and sysfs on xfs, nfs, bcache, and disk iostats. And make these package self contained so they do not bring in any additional dependencies. Also refactor the iostats package into a blockdevice package, and improve naming and organization of this package. Signed-off-by: Paul Gier <pgier@redhat.com>
This allows the client to determine whether the discard fields are valid. Signed-off-by: Paul Gier <pgier@redhat.com>
Signed-off-by: Paul Gier <pgier@redhat.com>
Signed-off-by: Paul Gier <pgier@redhat.com>
|
Rebased to get circleci build config changes. |
@grobie Yes, I think it would be good to completely remove the |
xfs/parse_test.go
Outdated
| @@ -11,23 +11,20 @@ | |||
| // See the License for the specific language governing permissions and | |||
| // limitations under the License. | |||
|
|
|||
| package xfs_test | |||
There was a problem hiding this comment.
A nit as the author of this package: I would prefer this to remain xfs_test so we are forced to exercise the exported API. It doesn't appear any unexported calls are used anyway.
There was a problem hiding this comment.
I agree this makes sense since we're trying to validate API usage. I have changed parse_test and xfs_test back to using the xfs_test package. I'll review some of the other packages to see where it makes sense to follow the same pattern.
Signed-off-by: Paul Gier <pgier@redhat.com>
Remove dependencies from procfs and sysfs onto xfs, nfs, bcache, and iostats
- Added a previously failing test - Fixes prometheus#136
Make the xfs and nfs packages dependent on procfs
instead of the procfs package dependent on xfs and nfs.
This allows clients to depend on procfs without bringing
in xfs and nfs.
This is a possibility for option 1 of issue #135