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 some enhancements to the volume replication CR #77

Merged
merged 5 commits into from May 19, 2021
Merged

Make some enhancements to the volume replication CR #77

merged 5 commits into from May 19, 2021

Conversation

iamniting
Copy link
Member

As part of the PR made below changes to the custom resource

  • Add validation for the spec.state
  • Add printcolumn
  • Add shortName

Signed-off-by: Nitin Goyal nigoyal@redhat.com

@iamniting iamniting requested review from sp98 and Madhu-1 April 30, 2021 11:50
@Madhu-1
Copy link
Member

Madhu-1 commented May 3, 2021

The commit message is not descriptive, Can you please add more details and also please squash the commits where it's not required.

@Madhu-1
Copy link
Member

Madhu-1 commented May 3, 2021

Please paste the output from the kube CLI if required

@iamniting
Copy link
Member Author

We can not create a CR with an invalid state

$ oc create -f file.yaml 
The VolumeReplication "vr1" is invalid: spec.replicationState: Unsupported value: "primar": supported values: "primary", "secondary", "resync"

Now it will print some informational field also

$ oc get volumereplication
NAME   AGE   REPLICATIONSTATE   STATUSSTATE   MESAGE
vr1    7s    primary

We can use vr to get the volumereplication CR's

$ oc get vr
NAME   AGE   REPLICATIONSTATE   STATUSSTATE   MESAGE
vr1    39s   primary

sp98
sp98 reviewed May 3, 2021
View reviewed changes
Copy link
Member

@sp98 sp98 left a comment

Choose a reason for hiding this comment

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

what do you think?

api/v1alpha1/volumereplication_types.go Outdated Show resolved Hide resolved
api/v1alpha1/volumereplication_types.go Outdated Show resolved Hide resolved
@iamniting
Copy link
Member Author

what do you think?

Like the idea and made changes

sp98
sp98 previously approved these changes May 3, 2021
View reviewed changes
@Madhu-1
Copy link
Member

Madhu-1 commented May 4, 2021

will merge this one once we branch out or create a fork for downstream

@Madhu-1
Copy link
Member

Madhu-1 commented May 4, 2021

@Mergifyio rebase

@iamniting
api: Add validation for the spec.state in volumereplication CR
93b4a76 
With this change user wont be able to create a CR with invalid state

Signed-off-by: Nitin Goyal <nigoyal@redhat.com>
@mergify
Copy link

mergify bot commented May 4, 2021

Command rebase: success

Branch has been successfully rebased

@mergify mergify bot dismissed sp98’s stale review May 4, 2021 09:22

Pull request has been modified.

@iamniting iamniting requested review from Madhu-1 and sp98 May 4, 2021 09:25
sp98
sp98 previously approved these changes May 19, 2021
View reviewed changes
@mergify mergify bot dismissed sp98’s stale review May 19, 2021 08:26

Pull request has been modified.

@iamniting
api: Add printcolumn for volumereplication CR
2930d65 
This will print columns while doing 'kubectl get vr'

Signed-off-by: Nitin Goyal <nigoyal@redhat.com>
@iamniting
api: Add shortName for volumereplication CR
ad567c3 
With this change we can get volumereplication with small name eg.
'kubectl get vr'

Signed-off-by: Nitin Goyal <nigoyal@redhat.com>
@iamniting
api: Add shortName for volumereplicationClass CR
8d1f31a 
With this change we can get volumereplicationclass with small name eg.
'kubectl get vrc'

Signed-off-by: Nitin Goyal <nigoyal@redhat.com>
@iamniting
api: Add printcolumn for volumereplicationclass CR
ad1f1ad 
This will print columns while doing 'kubectl get vrc'

Signed-off-by: Nitin Goyal <nigoyal@redhat.com>
Madhu-1
Madhu-1 reviewed May 19, 2021
View reviewed changes
Copy link
Member

@Madhu-1 Madhu-1 left a comment

Choose a reason for hiding this comment

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

LGTM. @iamniting can you paste the output how it looks. If everything looks good we can merge this PR.

@iamniting
Copy link
Member Author 

$ oc get vrc
NAME   PROVISIONER
vrc    rook-ceph.rbd.csi.ceph.com

$ oc get vr
NAME   AGE   VOLUMEREPLICATIONCLASS   PVCNAME   DESIREDSTATE   CURRENTSTATE
vr1    23s   vrc                      pvc1      primary

@Madhu-1 Madhu-1 added the ready-to-merge This PR is ready to be merged and it doesn't need second review label May 19, 2021
@Madhu-1
Copy link
Member

Madhu-1 commented May 19, 2021

@Mergifyio refresh

@mergify
Copy link

mergify bot commented May 19, 2021

Command refresh: success

Pull request refreshed

@Madhu-1
Copy link
Member

Madhu-1 commented May 19, 2021

Looks like i need to revisit mergify rules , Merging this one manually

@Madhu-1 Madhu-1 merged commit 4a9f87e into csi-addons:main May 19, 2021
@Madhu-1 Madhu-1 added the backport-to-release-v0.1 required for backport label May 24, 2021
@Madhu-1
Copy link
Member

Madhu-1 commented May 24, 2021

@Mergifyio refresh

@mergify
Copy link

mergify bot commented May 24, 2021

Command refresh: success

Pull request refreshed

@mergify mergify bot mentioned this pull request May 24, 2021
Merged
@openshift-ci openshift-ci bot mentioned this pull request May 26, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Madhu-1 Madhu-1 Madhu-1 approved these changes

@sp98 sp98 sp98 left review comments

Assignees
No one assigned
Labels
backport-to-release-v0.1 required for backport ready-to-merge This PR is ready to be merged and it doesn't need second review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@iamniting @Madhu-1 @sp98