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

make the formatting errors based on go 1.19 #758

Merged
merged 1 commit into from
Sep 13, 2022
Merged

make the formatting errors based on go 1.19 #758

merged 1 commit into from
Sep 13, 2022

Conversation

humblec
Copy link
Contributor

@humblec humblec commented Sep 6, 2022 •
edited
Loading

Signed-off-by: Humble Chirammal hchiramm@redhat.com

/kind cleanup

NONE

Additional Note for reviewer:

The errors in absence of this PR cause tests to fail in kubernetes-csi/csi-release-tools#203

@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 Sep 6, 2022
@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Sep 6, 2022
@humblec humblec force-pushed the correct-fmt branch 2 times, most recently from 174c571 to b16bd40 Compare September 6, 2022 10:25
pohly
pohly reviewed Sep 6, 2022
View reviewed changes
go.mod Outdated Show resolved Hide resolved
pohly
pohly reviewed Sep 6, 2022
View reviewed changes
vendor/github.com/kubernetes-csi/external-snapshotter/client/v6/apis/volumesnapshot/v1/types.go Outdated
// - VolumsnapshotStatus is used by end users because they cannot see VolumeSnapshotContent.
// - CSI snapshotter sidecar is light weight as it only watches VolumeSnapshotContent
// object, not VolumeSnapshot object.
// - Fields in VolumeSnapshotContentStatus can be used for filtering when importing a
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does the code under vendor change?

You said "lift the dependencies" - what exactly does that mean?

I don't see a version change in go.mod.

Copy link
Contributor Author

@humblec humblec Sep 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I was referrring the update in client go.mod with version bump to kube v1.25 https://github.com/kubernetes-csi/external-snapshotter/pull/758/files#diff-de2c00fe7d49a7bb4d5c225eded980780a7b9dde4ae89e70c7e497cd0e45b15bL9-R6 by lift the dependencies.

@xing-yang
Copy link
Collaborator

Please don't update anything in the client folder yet.
/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 6, 2022
@humblec humblec changed the title lift the dependencies and correct format errors based on 1.19 [WIP] lift the dependencies and correct format errors based on 1.19 Sep 7, 2022
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 7, 2022
@pohly
Copy link
Contributor

pohly commented Sep 7, 2022

IMHO all that this PR should do is make the formatting compliant with Go 1.19 and nothing else. Then we can merge kubernetes-csi/csi-release-tools#203.

@humblec
Copy link
Contributor Author

humblec commented Sep 7, 2022

IMHO all that this PR should do is make the formatting compliant with Go 1.19 and nothing else. Then we can merge kubernetes-csi/csi-release-tools#203.

ok.. let me shrink the PR to just do formatting based on 1.19 in that case.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 7, 2022
@humblec humblec changed the title [WIP] lift the dependencies and correct format errors based on 1.19 make the formatting errors based on go 1.19 Sep 7, 2022
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 7, 2022
@humblec
Copy link
Contributor Author

humblec commented Sep 7, 2022

@pohly @xing-yang as per the above discussion, I have kept only formatting change here in this PR. ptal.. Thanks..

@humblec humblec requested review from pohly and removed request for Jiawei0227 September 7, 2022 06:39
pohly
pohly suggested changes Sep 7, 2022
View reviewed changes
pkg/common-controller/snapshot_controller.go Outdated Show resolved Hide resolved
pkg/sidecar-controller/snapshot_controller.go Outdated Show resolved Hide resolved
@xing-yang
Copy link
Collaborator

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 7, 2022
@humblec humblec requested a review from pohly September 13, 2022 07:28
@pohly
Copy link
Contributor

pohly commented Sep 13, 2022

Can you remove the dependency update commit? It is unrelated to this PR.

@pohly
Copy link
Contributor

pohly commented Sep 13, 2022

You probably added it because of a Trivy failure, but that's just bad style. I'm really getting annoyed by this security check.

@humblec
Copy link
Contributor Author

humblec commented Sep 13, 2022

Can you remove the dependency update commit? It is unrelated to this PR.

The Trivvy scnner fail here in this PR on lastest run, so clubbed with the same. I can split that to another PR too and mark it as dependency for this PR.
@pohly wdyt?

@humblec
Copy link
Contributor Author

humblec commented Sep 13, 2022

You probably added it because of a Trivy failure, but that's just bad style. I'm really getting annoyed by this security check.

Indeed . Thats why I refreshed with that change. Let me split that up first.

@humblec
Copy link
Contributor Author

humblec commented Sep 13, 2022 •
edited
Loading

The trrivvy failure has been addressed here #759

@humblec
make the formatting errors based on go 1.19
d34102f 
Reference#
kubernetes-csi/csi-release-tools#203

Signed-off-by: Humble Chirammal <hchiramm@redhat.com>
@humblec
Copy link
Contributor Author

humblec commented Sep 13, 2022

The trrivvy failure has been addressed here #759

As the trivvy failure has been addressed on master, I have rebased this PR. This should be good to go as well @xing-yang @pohly

pohly
pohly approved these changes Sep 13, 2022
View reviewed changes
Copy link
Contributor

@pohly pohly left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 13, 2022
@xing-yang
Copy link
Collaborator

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: humblec, pohly, xing-yang

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

The pull request process is described 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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 13, 2022
@k8s-ci-robot k8s-ci-robot merged commit 7c1ca79 into kubernetes-csi:master Sep 13, 2022
@humblec humblec mentioned this pull request Sep 14, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pohly pohly pohly approved these changes

Assignees

@pohly pohly

@xing-yang xing-yang

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. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. 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.
4 participants
@humblec @xing-yang @pohly @k8s-ci-robot