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.

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

FSType is now optional in Volume objects and 'ext4' used when it's when omitted. #19921

Merged
merged 1 commit into from
Feb 10, 2016
Merged

FSType is now optional in Volume objects and 'ext4' used when it's when omitted. #19921

merged 1 commit into from
Feb 10, 2016

Conversation

jsafrane
Copy link
Member

This is follow up of #17650 (comment).

Most volume plugins use SafeFormatAndMount, which uses ext4 by default.

FlexVolume plugin has FSType attribute 'omitempty', so reflect it in the
description of the type.

Also, update AWS EBS plugin to have the same style as the rest of plugins - see #17650 (comment)

@swagiaal, would you mind a short review?

@k8s-github-robot
Copy link

Labelling this PR as size/M

@k8s-github-robot k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 21, 2016
ghost
ghost reviewed Jan 21, 2016
View reviewed changes
pkg/volume/aws_ebs/aws_ebs.go
@@ -110,7 +110,7 @@ func (plugin *awsElasticBlockStorePlugin) newBuilderInternal(spec *volume.Spec,
fsType: fsType,
partition: partition,
readOnly: readOnly,
diskMounter: &mount.SafeFormatAndMount{plugin.host.GetMounter(), exec.New()}}, nil
diskMounter: &mount.SafeFormatAndMount{mounter, exec.New()}}, nil
Copy link

Choose a reason for hiding this comment

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

Could you separate this change into its own PR it is not related to this PR.

@k8s-bot
Copy link

k8s-bot commented Jan 21, 2016

GCE e2e test build/test passed for commit 10ad5c669279ed591306f9c7ac8419959e354b58.

@ghost
Copy link

ghost commented Jan 21, 2016

@jsafrane thanks for following up...
The validation changes look good to me.
The api doc changes look good to but you will have to do them in v1/types.go as well.
The aws ebs change looks good but it should be in a separate PR

@k8s-bot
Copy link

k8s-bot commented Jan 21, 2016

GCE e2e test build/test passed for commit ca9fc5e3399b5b8758666b4ea50e8db93229d9cc.

@jsafrane
Copy link
Member Author

you will have to do them in v1/types.go as well.

Will do shortly.

The aws ebs change looks good but it should be in a separate PR

Separate commit is not enough?

@k8s-github-robot
Copy link

The author of this PR is not in the whitelist for merge, can one of the admins add the 'ok-to-merge' label?

@ghost
Copy link

ghost commented Jan 21, 2016

Separate commit is not enough?

I think it should be a separate PR. Separate commits are for related sequential changes building up to one big modification.

@k8s-github-robot k8s-github-robot removed the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 21, 2016
@k8s-github-robot
Copy link

Labelling this PR as size/L

@k8s-github-robot k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 21, 2016
@k8s-bot
Copy link

k8s-bot commented Jan 21, 2016

GCE e2e test build/test passed for commit 1919b03e145baef2d36c354b8f744059f3716abd.

@jsafrane
Copy link
Member Author

Ok, I removed AWS changes from the PR. I guess we'll sort it out when we rewrite plugins with the new Attach interface.

@k8s-bot
Copy link

k8s-bot commented Jan 22, 2016

GCE e2e test build/test passed for commit 6196de61b974597fb64b59f236ddc1bb6bee91e8.

@ghost
Copy link

ghost commented Jan 22, 2016

Hmm.. the aws change seems to still be there
Doc changes look good

@jsafrane
Copy link
Member Author

Weird, I would swear I removed the patch. Now it's really removed.

@k8s-bot
Copy link

k8s-bot commented Jan 22, 2016

GCE e2e test build/test passed for commit bb0728d1801f300d7a5184667dbc34ac39ce468c.

@ghost
Copy link

ghost commented Jan 22, 2016

@jsafrane thanks... LGTM

markturansky
markturansky reviewed Jan 22, 2016
View reviewed changes
pkg/api/types.go
// Must be a filesystem type supported by the host operating system.
// Ex. "ext4", "xfs", "ntfs"
// Ex. "ext4", "xfs", "ntfs". The default one depends on FlexVolume script.
Copy link
Contributor

Choose a reason for hiding this comment

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

"The default filesystem depends on ..."

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated, thanks.

@markturansky
Copy link
Contributor

LGTM after the wording changes above.

@bgrant0607 please review for API changes?

@googlebot googlebot added cla: yes and removed cla: no labels Feb 1, 2016
@jsafrane jsafrane changed the title Remove validation of *VolumeSource.FSType. FSType is now optional in Volume objects and 'ext4' used when it's when omitted. Feb 1, 2016
@k8s-bot
Copy link

k8s-bot commented Feb 1, 2016

GCE e2e test build/test passed for commit f1c1cd90bbbe261e002b96f420527f261b3b67b6.

@k8s-github-robot
Copy link

PR needs rebase

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 2, 2016
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 2, 2016
@k8s-bot
Copy link

k8s-bot commented Feb 2, 2016

GCE e2e test build/test passed for commit 6e64dc9ec4cb5452d4705ab3336f02b70482a519.

@k8s-github-robot
Copy link

PR needs rebase

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 9, 2016
@jsafrane
Remove validation of *VolumeSource.FSType.
40c97dd 
Most volume plugins use SafeFormatAndMount, which uses ext4 by default.

FlexVolume plugin has FSType attribute 'omitempty', so reflect it in the
description of the type.
@jsafrane
Copy link
Member Author

jsafrane commented Feb 9, 2016

rebased

@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 9, 2016
@k8s-bot
Copy link

k8s-bot commented Feb 9, 2016

GCE e2e test build/test passed for commit 40c97dd.

@bgrant0607 bgrant0607 mentioned this pull request Feb 10, 2016
Closed
@bgrant0607
Copy link
Member

LGTM

@bgrant0607 bgrant0607 added lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-merge labels Feb 10, 2016
@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Feb 10, 2016

GCE e2e test build/test passed for commit 40c97dd.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

k8s-github-robot pushed a commit that referenced this pull request Feb 10, 2016
Merge pull request #19921 from jsafrane/devel/remove-fstype-check
36c25be 
Auto commit by PR queue bot
@k8s-github-robot k8s-github-robot merged commit 36c25be into kubernetes:master Feb 10, 2016
@david-mcmahon david-mcmahon added the release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. label Apr 8, 2016
@bgrant0607 bgrant0607 mentioned this pull request May 11, 2016
Closed
@jsafrane jsafrane deleted the devel/remove-fstype-check branch August 19, 2016 11:11
@jsafrane jsafrane mentioned this pull request Aug 28, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@bgrant0607 bgrant0607

Labels
kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@jsafrane @k8s-github-robot @k8s-bot @markturansky @bgrant0607 @googlebot @k8s-teamcity-mesosphere @smarterclayton @david-mcmahon