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

mountinfo: use idiomatic naming for fields #34

Merged
merged 1 commit into from
Oct 2, 2020

Conversation

thaJeztah
Copy link
Member

follow-up to #32

I am looking at using these modules in containerd, so that we can reduce the effort of maintaining "mountinfo" implementations.

This change renames the info fields to match the ones used in containerd, which are more "correct" from a Go naming perspective: containerd/containerd@8cd2182

This will be a breaking changes for consumers of this package, but we're still pre-v1.0.0, so if we want to make these changes, this is still possible now.

@kolyshkin
Copy link
Collaborator

I tend to agree the new names are better, and guess we can convert existing users. 👍

@kolyshkin
Copy link
Collaborator

This will only be problematic for repos which use bots to update dependencies (and only if their maintainers ignore CI failures)

@kolyshkin
Copy link
Collaborator

Can you please rename FstypeFilter while at it? I named it this way because the of the field name we're changing.

@kolyshkin
Copy link
Collaborator

Alternatively, of course, we can modify containerd to expect these fields. I'm not sure which way is better.

@thaJeztah thaJeztah force-pushed the idiomatic_naming branch 2 times, most recently from b5f7760 to 7b71b2d Compare October 1, 2020 21:47
I am looking at using these modules in containerd, so that we can
reduce the effort of maintaining "mountinfo" implementations.

This change renames the info fields to match the ones used in
containerd, which are more "correct" from a Go naming perspective:

containerd/containerd@8cd2182

This will be a breaking changes for consumers of this package, but
we're still pre-v1.0.0, so if we want to make these changes, this
is still possible now.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah marked this pull request as ready for review October 1, 2020 22:40
@thaJeztah
Copy link
Member Author

Rebased 👍 ptal

Copy link
Collaborator

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

LGTM

We need to cut a release with this change and convert all the users.

@kolyshkin kolyshkin merged commit a03c53e into moby:master Oct 2, 2020
@kolyshkin
Copy link
Collaborator

Can you please rename FstypeFilter while at it? I named it this way because the of the field name we're changing.

Ah, we forgot about this one, and surely I found it a minute after I cut a release :(

@thaJeztah
Copy link
Member Author

Ah, we forgot about this one, and surely I found it a minute after I cut a release :(

oh! Sorry, good catch, looks like I missed that comment; thanks for fixing in #41

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