Skip to content

Change Fstype default to emptyString from ext4#358

Merged
k8s-ci-robot merged 1 commit intokubernetes-csi:masterfrom
Kartik494:emptydefaultfs
Jun 16, 2022
Merged

Change Fstype default to emptyString from ext4#358
k8s-ci-robot merged 1 commit intokubernetes-csi:masterfrom
Kartik494:emptydefaultfs

Conversation

@Kartik494
Copy link
Copy Markdown

@Kartik494 Kartik494 commented Jun 1, 2022 •
edited by jsafrane
Loading

What type of PR is this?
/kind bug

Which issue(s) this PR fixes:

Fixes #343

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Change `--default-fstype` to empty string. Action required: CSI drivers that depended on ext4 as the default need to change their deployments to explicitly set `--default-fstype=ext4`.

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 1, 2022
@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jun 1, 2022
@Kartik494
Copy link
Copy Markdown
Author

Kartik494 commented Jun 1, 2022

/cc @RomanBednar

@k8s-ci-robot k8s-ci-robot requested a review from RomanBednar June 1, 2022 10:13
@Kartik494
Copy link
Copy Markdown
Author

Kartik494 commented Jun 1, 2022

Is this make sense to change fsType to empty string in test case also?

external-attacher/pkg/controller/csi_handler_test.go

Line 47 in 5545207

defaultFSType = "ext4"

Kartik494
Kartik494 commented Jun 1, 2022
View reviewed changes
Comment thread cmd/csi-attacher/main.go Outdated
leaderElectionRetryPeriod = flag.Duration("leader-election-retry-period", 5*time.Second, "Duration, in seconds, the LeaderElector clients should wait between tries of actions. Defaults to 5 seconds.")

defaultFSType = flag.String("default-fstype", "ext4", "The default filesystem type of the volume to publish.")
defaultFSType = flag.String("default-fstype", "", "The default filesystem type of the volume to publish.")
Copy link
Copy Markdown
Author

@Kartik494 Kartik494 Jun 1, 2022

Choose a reason for hiding this comment

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

Is extending description as Default to empty string make sense?

Copy link
Copy Markdown
Contributor

@ggriffiths ggriffiths Jun 9, 2022

Choose a reason for hiding this comment

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

Yes, Defaults to empty string would make sense here

Copy link
Copy Markdown
Author

@Kartik494 Kartik494 Jun 9, 2022

Choose a reason for hiding this comment

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

Done

@Kartik494
Copy link
Copy Markdown
Author

Kartik494 commented Jun 1, 2022

/kind bug

@k8s-ci-robot k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Jun 1, 2022
@mauriciopoppe
Copy link
Copy Markdown
Member

mauriciopoppe commented Jun 1, 2022

/cc @jingxu97

@k8s-ci-robot k8s-ci-robot requested a review from jingxu97 June 1, 2022 16:28
@ggriffiths
Copy link
Copy Markdown
Contributor

ggriffiths commented Jun 8, 2022

Is this make sense to change fsType to empty string in test case also?

external-attacher/pkg/controller/csi_handler_test.go

Line 47 in 5545207

defaultFSType = "ext4"

Yes, let's test with the same default.

@ggriffiths
Copy link
Copy Markdown
Contributor

ggriffiths commented Jun 8, 2022

@Kartik494 can you add a release note for this as we're changing a default value?

@jingxu97
Copy link
Copy Markdown

jingxu97 commented Jun 8, 2022

Just want to check whether it will have any behavior change from this?

@Kartik494
Copy link
Copy Markdown
Author

Kartik494 commented Jun 9, 2022

/release-note Change Fstype default to emptyString

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Jun 9, 2022
@Kartik494
Copy link
Copy Markdown
Author

Kartik494 commented Jun 9, 2022

@Kartik494 can you add a release note for this as we're changing a default value?

Done

@Kartik494
Copy link
Copy Markdown
Author

Kartik494 commented Jun 9, 2022

Is this make sense to change fsType to empty string in test case also?

external-attacher/pkg/controller/csi_handler_test.go

Line 47 in 5545207

defaultFSType = "ext4"

Yes, let's test with the same default.

Thanks for the inputs, i will modify this.

@jsafrane
Copy link
Copy Markdown
Contributor

jsafrane commented Jun 9, 2022

@Kartik494 please add Action needed: to the release note, and mention that drivers that depended on ext4 as the default need to change their deployments to explicitly set ext4.

And IMO we will need to release 4.0 instead of 3.6.

@k8s-ci-robot k8s-ci-robot added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jun 16, 2022
@Kartik494
Copy link
Copy Markdown
Author

Kartik494 commented Jun 16, 2022 •
edited
Loading

@Kartik494 please add Action needed: to the release note, and mention that drivers that depended on ext4 as the default need to change their deployments to explicitly set ext4.

And IMO we will need to release 4.0 instead of 3.6.

Done
Thanks!!

@Kartik494
Copy link
Copy Markdown
Author

Kartik494 commented Jun 16, 2022

Just want to check whether it will have any behavior change from this?

Seems Like empty string will not change in any behaviour change but disable the interference.
Please see #342 (comment)

@jsafrane
Copy link
Copy Markdown
Contributor

jsafrane commented Jun 16, 2022

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 16, 2022
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Jun 16, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsafrane, Kartik494

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 16, 2022
@k8s-ci-robot k8s-ci-robot merged commit 438ddbd into kubernetes-csi:master Jun 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ggriffiths ggriffiths Awaiting requested review from ggriffiths

@humblec humblec Awaiting requested review from humblec

@RomanBednar RomanBednar Awaiting requested review from RomanBednar

@jingxu97 jingxu97 Awaiting requested review from jingxu97

Assignees

@jsafrane jsafrane

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

change default fstype from "ext4" to empty string

6 participants

@Kartik494 @mauriciopoppe @ggriffiths @jingxu97 @jsafrane @k8s-ci-robot