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

Addressed few todo's #4268

Merged
merged 3 commits into from
Nov 23, 2023
Merged

Addressed few todo's #4268

merged 3 commits into from
Nov 23, 2023

Conversation

riya-singhal31
Copy link
Contributor

This PR updates the go-ceph version to use github.com/ceph/go-ceph/cephfs and fixed few todo by replacing the string comparison with error code check.

nixpanic
nixpanic previously approved these changes Nov 21, 2023
View reviewed changes
@nixpanic nixpanic requested a review from a team November 21, 2023 12:42
@nixpanic nixpanic added component/cephfs Issues related to CephFS rebase update the version of an external component dependency/go-ceph depends on go-ceph functionality labels Nov 21, 2023
@mergify mergify bot dismissed nixpanic’s stale review November 21, 2023 14:21

Pull request has been modified.

nixpanic
nixpanic reviewed Nov 21, 2023
View reviewed changes
deploy/cephcsi/image/Dockerfile Outdated
@@ -38,6 +38,7 @@ RUN ${GOROOT}/bin/go version && ${GOROOT}/bin/go env

RUN dnf -y install --nodocs \
librados-devel librbd-devel \
librados-devel libcephfs-devel \
Copy link
Member

Choose a reason for hiding this comment

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

librados-devel is now listed 2x. Just add libcephfs-devel on the same line as librbd-devel

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated, thanks Niels!

nixpanic
nixpanic reviewed Nov 21, 2023
View reviewed changes
scripts/Dockerfile.devel Outdated
@@ -29,6 +29,7 @@ RUN dnf -y install \
gcc \
librados-devel \
librbd-devel \
libcephfs-devel \
Copy link
Member

Choose a reason for hiding this comment

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

Can you check the indention?

Copy link
Contributor Author

@riya-singhal31 riya-singhal31 Nov 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Locally the earlier one was fine, adding an extra tab makes it properly indent here. So added now.

nixpanic
nixpanic reviewed Nov 21, 2023
View reviewed changes
scripts/Dockerfile.test Outdated
@@ -33,6 +33,7 @@ RUN source /build.env \
findutils \
librados-devel \
librbd-devel \
libcephfs-devel \
Copy link
Member

Choose a reason for hiding this comment

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

and here as well?

@nixpanic
Copy link
Member

@Mergifyio rebase

@riya-singhal31 @nixpanic
rebase: update github.com/ceph/go-ceph to master
2a0eeb3 
this commit updates the version of go-ceph imported to
latest, so we can make use of github.com/ceph/go-ceph/cephfs

Signed-off-by: Riya Singhal <[email protected]>
@riya-singhal31 @nixpanic
util: addresed few todo
d04dfbb 
this commit replaces string comparsion with error code
at few places

Signed-off-by: Riya Singhal <[email protected]>
@riya-singhal31 @nixpanic
rebase: install libcephfs-devel
3f2804e 
Signed-off-by: Riya Singhal <[email protected]>
@mergify Mergify
Copy link
Contributor

mergify bot commented Nov 22, 2023

rebase

✅ Branch has been successfully rebased

@nixpanic
Copy link
Member

@Mergifyio queue

@mergify Mergify
Copy link
Contributor

mergify bot commented Nov 22, 2023 •
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.

@mergify mergify bot added the ok-to-test Label to trigger E2E tests label Nov 22, 2023
@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.26

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/upgrade-tests-cephfs

@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.26

@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/k8s-1.28

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/upgrade-tests-rbd

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot ceph-csi-bot removed the ok-to-test Label to trigger E2E tests label Nov 22, 2023
@riya-singhal31
Copy link
Contributor Author

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

@mergify mergify bot merged commit 5e80c6a into ceph:devel Nov 23, 2023
24 checks passed
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

Assignees
No one assigned
Labels
component/cephfs Issues related to CephFS dependency/go-ceph depends on go-ceph functionality rebase update the version of an external component
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@riya-singhal31 @nixpanic @ceph-csi-bot @yati1998