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

Deprecate unused field MaxFileSize #83690

Closed
wants to merge 1 commit into from

Conversation

@tedyu
Copy link
Contributor

commented Oct 9, 2019

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
As #83689 pointed out, MaxFileSize field is not used.

This PR deprecates the field, referring to SupportedSizeRange.

Which issue(s) this PR fixes:
Fixes #83689

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


@@ -167,6 +167,7 @@ type DriverInfo struct {
FeatureTag string // FeatureTag for the driver

// Max file size to be tested for this driver
// Deprecated - see SupportedSizeRange

This comment has been minimized.

Copy link
@davidz627

davidz627 Oct 9, 2019

Contributor

could you also remove all usage of this field

@yutedz yutedz force-pushed the yutedz:driver-info-max-size branch from 66f804b to 8f0cb09 Oct 9, 2019
@k8s-ci-robot k8s-ci-robot added size/M and removed size/XS labels Oct 9, 2019
@davidz627

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2019

/priority backlog
/assign @msau42

@davidz627

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2019

@k8s-ci-robot k8s-ci-robot requested review from hoyho and jsafrane Oct 9, 2019
@davidz627

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2019

/assign

@yutedz yutedz force-pushed the yutedz:driver-info-max-size branch from 8f0cb09 to 0fe66de Oct 10, 2019
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Oct 10, 2019

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: tedyu
To complete the pull request process, please assign davidz627
You can assign the PR to them by writing /assign @davidz627 in a comment when ready.

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

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

@yutedz yutedz force-pushed the yutedz:driver-info-max-size branch from 0fe66de to f0f5afc Oct 10, 2019
SupportedFsType: sets.NewString(
"", // Default fsType
),
SupportedSizeRange: volume.SizeRange{
Min: "1Mi",
Max: strconv.FormatInt(testpatterns.FileSizeMedium, 10),

This comment has been minimized.

Copy link
@hoyho

hoyho Oct 10, 2019

Member

I think hostPath driver should not have a max size limitation

This comment has been minimized.

Copy link
@tedyu

tedyu Oct 10, 2019

Author Contributor

this was translated from original config:

                       MaxFileSize: testpatterns.FileSizeMedium,
SupportedSizeRange: volume.SizeRange{
Min: "5Gi",
Max: strconv.FormatInt(testpatterns.FileSizeMedium, 10),

This comment has been minimized.

Copy link
@hoyho

hoyho Oct 10, 2019

Member

Looks like testpatterns.FileSizeMedium will return type int64 but the size here should be a string like "10Gi", not in KB

SupportedSizeRange: volume.SizeRange{
Min: "5Gi",
Max: strconv.FormatInt(testpatterns.FileSizeLarge, 10),

This comment has been minimized.

Copy link
@hoyho

hoyho Oct 10, 2019

Member

Why set Max for all driver? There's many similar operation to other drivers

@yutedz yutedz force-pushed the yutedz:driver-info-max-size branch from f0f5afc to cf7d6ff Oct 10, 2019
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Oct 10, 2019

@tedyu: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-e2e-gce cf7d6ff link /test pull-kubernetes-e2e-gce
pull-kubernetes-e2e-gce-storage-slow cf7d6ff link /test pull-kubernetes-e2e-gce-storage-slow
pull-kubernetes-e2e-gce-csi-serial cf7d6ff link /test pull-kubernetes-e2e-gce-csi-serial
pull-kubernetes-verify cf7d6ff link /test pull-kubernetes-verify

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@tedyu tedyu closed this Oct 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.