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

ci: Update golangci lint and helm version #4539

Merged
merged 15 commits into from
Apr 9, 2024
Merged

ci: Update golangci lint and helm version #4539

merged 15 commits into from
Apr 9, 2024

Conversation

Madhu-1
Copy link
Collaborator

@Madhu-1 Madhu-1 commented Apr 4, 2024

Updated golangci-lint and helm to the latest release and addressed all the linter issues.

@Madhu-1 Madhu-1 requested review from a team April 4, 2024 09:22
@mergify mergify bot added component/testing Additional test cases or CI work component/deployment Helm chart, kubernetes templates and configuration Issues/PRs labels Apr 4, 2024
nixpanic
nixpanic previously approved these changes Apr 4, 2024
View reviewed changes
internal/cephfs/core/clone_test.go
"testing"

cerrors "github.com/ceph/ceph-csi/internal/cephfs/errors"

fsa "github.com/ceph/go-ceph/cephfs/admin"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
Copy link
Member

@nixpanic nixpanic Apr 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

nit: no need to change to require, could have stayed assert as that also provides ErrorIs() and there are is no dependent usage of the result. Not sure what golangci-lint reported about this though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

golangci lint suggested to use require instead of assert and it was failing, so i made this change

Copy link
Member

Choose a reason for hiding this comment

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

can github.com/stretchr/testify/assert be removed from go.mod and vendor/ after these changes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i assume if its not required, CI would have caught it, will check and address it if required.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, I'm merely wondering about it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

[🎩︎]mrajanna@li-2cfbef4c-22d9-11b2-a85c-a3e4a93c405f ceph-csi $]go mod tidy
[🎩︎]mrajanna@li-2cfbef4c-22d9-11b2-a85c-a3e4a93c405f ceph-csi $]git diff

i did run go mod tidy didnt find anything as we have github.com/stretchr/testify as import in go.mod

@nixpanic nixpanic requested a review from a team April 4, 2024 09:41
@Madhu-1 Madhu-1 requested a review from iPraveenParihar April 4, 2024 12:55
Rakshith-R
Rakshith-R reviewed Apr 5, 2024
View reviewed changes
Copy link
Contributor

@Rakshith-R Rakshith-R left a comment

Choose a reason for hiding this comment

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

just one small nit.

e2e/rbd_helper.go Outdated Show resolved Hide resolved
internal/cephfs/core/filesystem.go
@@ -29,11 +29,11 @@ import (
// that interacts with CephFS filesystem API's.
type FileSystem interface {
// GetFscID returns the ID of the filesystem with the given name.
GetFscID(context.Context, string) (int64, error)
GetFscID(ctx context.Context, fsName string) (int64, error)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please point to the linter that suggested this change?
I am interested in reading about the suggestion.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

internal/cephfs/core/filesystem.go:32:11: interface method GetFscID must have named param for type context.Context (inamedparam)
	GetFscID(context.Context, string) (int64, error)
	         ^

@Madhu-1 Madhu-1 force-pushed the update-golangci-lint branch from 6c24b3c to fb407b8 Compare April 5, 2024 09:28
@mergify mergify bot dismissed nixpanic’s stale review April 5, 2024 09:29

Pull request has been modified.

@Madhu-1 Madhu-1 requested review from Rakshith-R and nixpanic April 5, 2024 09:35
nixpanic
nixpanic previously approved these changes Apr 5, 2024
View reviewed changes
Rakshith-R
Rakshith-R previously approved these changes Apr 5, 2024
View reviewed changes
@Madhu-1
Copy link
Collaborator Author

Madhu-1 commented Apr 5, 2024

@Mergifyio queue

@mergify Mergify
Copy link
Contributor

mergify bot commented Apr 5, 2024 •
edited
Loading

queue

🛑 The pull request has been removed from the queue default

The queue conditions cannot be satisfied due to failing checks.

You can take a look at Queue: Embarked in merge queue check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

@Madhu-1 Madhu-1 force-pushed the update-golangci-lint branch from fb407b8 to 2e36288 Compare April 5, 2024 14:01
@mergify mergify bot added the ok-to-test Label to trigger E2E tests label Apr 5, 2024
@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.27

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.27

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.29

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.28

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.27

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.29

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.28

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.29

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.28

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/upgrade-tests-cephfs

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/upgrade-tests-rbd

@ceph-csi-bot ceph-csi-bot removed the ok-to-test Label to trigger E2E tests label Apr 5, 2024
Madhu-1 added 3 commits April 8, 2024 08:20
@Madhu-1
ci: update helm to latest release
1d6f921 
updating helm to the latest release
3.14.3

Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
@Madhu-1
ci: use timeout for golangci-lint
be418b0 
deadline is replaced with timeout
in recent release and updated the
reference to the new sample yaml.

Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
@Madhu-1
cleanup: remove nolint:interfacer comment
c4ce2b3 
nolint:interfacer is not required for
the latest golangci-lint

Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
@Madhu-1 Madhu-1 force-pushed the update-golangci-lint branch from 2e36288 to c4ce2b3 Compare April 8, 2024 06:20
@mergify mergify bot dismissed stale reviews from Rakshith-R and nixpanic April 8, 2024 06:21

Pull request has been modified.

@Madhu-1 Madhu-1 added the ok-to-test Label to trigger E2E tests label Apr 8, 2024
@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.28

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.27

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.28

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.27

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/upgrade-tests-cephfs

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.29

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.28

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.27

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/upgrade-tests-rbd

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.29

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.29

@Madhu-1 Madhu-1 requested review from nixpanic and Rakshith-R April 8, 2024 12:56
@ceph-csi-bot ceph-csi-bot removed the ok-to-test Label to trigger E2E tests label Apr 8, 2024
@Madhu-1 Madhu-1 requested a review from yati1998 April 8, 2024 12:58
@nixpanic
Copy link
Member

nixpanic commented Apr 9, 2024

@Mergifyio rebase

@mergify Mergify
Copy link
Contributor

mergify bot commented Apr 9, 2024

rebase

✅ Nothing to do for rebase action

@nixpanic
Copy link
Member

nixpanic commented Apr 9, 2024

@Mergifyio queue

@mergify Mergify
Copy link
Contributor

mergify bot commented Apr 9, 2024 •
edited
Loading

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 5aace6e

@mergify mergify bot merged commit 5aace6e into ceph:devel Apr 9, 2024
35 checks passed
@nixpanic nixpanic mentioned this pull request Apr 11, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yati1998 yati1998 yati1998 approved these changes

@nixpanic nixpanic nixpanic approved these changes

@iPraveenParihar iPraveenParihar Awaiting requested review from iPraveenParihar

@Rakshith-R Rakshith-R Awaiting requested review from Rakshith-R

Assignees
No one assigned
Labels
component/deployment Helm chart, kubernetes templates and configuration Issues/PRs component/testing Additional test cases or CI work
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Madhu-1 @ceph-csi-bot @nixpanic @yati1998 @Rakshith-R