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

remove IsUnstructuredCRDReady #4085

Merged
merged 1 commit into from
Sep 1, 2021

Conversation

reasonerjt
Copy link
Contributor

Please add a summary of your change

This commit removes IsUnstructuredCRDReady since
kubernetes/kubernetes#87675 is fixed.
It uses Is1CRDReady to check the readiness of CRD.

After v1.7 we may consider merge the funcx IsV1Beta1CRDReady and
IsV1CRDReady

Does your change fix a particular issue?

Fixes #4059

Please indicate you've done the following:

@reasonerjt reasonerjt added the kind/changelog-not-required PR does not require a user changelog. Often for docs, website, or build changes label Aug 31, 2021
@reasonerjt reasonerjt requested a review from ywk253100 August 31, 2021 16:02
@github-actions github-actions bot requested review from dsu-igeek and sseago August 31, 2021 16:02
@reasonerjt reasonerjt requested a review from zubron August 31, 2021 16:02
@reasonerjt reasonerjt force-pushed the rm-IsUnstructuredCRDReady branch from 76b864b to a58469f Compare August 31, 2021 16:03
zubron
zubron previously approved these changes Aug 31, 2021
Copy link
Contributor

@zubron zubron left a comment

Choose a reason for hiding this comment

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

LGTM - thanks! I'll approve again once the CI check is green (looks like a go fmt issue).

sseago
sseago previously approved these changes Aug 31, 2021
This commit removes `IsUnstructuredCRDReady` since
kubernetes/kubernetes#87675 is fixed.
Is uses `Is1CRDReady` to check the readiness of CRD.

After v1.7 we may consider merge the funcx `IsV1Beta1CRDReady` and
`IsV1CRDReady`

Signed-off-by: Daniel Jiang <jiangd@vmware.com>
@reasonerjt reasonerjt dismissed stale reviews from sseago and zubron via e23cbda September 1, 2021 01:08
@reasonerjt reasonerjt force-pushed the rm-IsUnstructuredCRDReady branch from a58469f to e23cbda Compare September 1, 2021 01:08
@jenting
Copy link
Contributor

jenting commented Sep 1, 2021

The upstream fixes this issue at Kubernetes 1.20+?
If yes, will this change impact the user using the Kubernetes < 1.20 to do restore?

@reasonerjt
Copy link
Contributor Author

reasonerjt commented Sep 1, 2021

The upstream fixes this issue at Kubernetes 1.20+?
If yes, will this change impact the user using the Kubernetes < 1.20 to do restore?

@jenting
I think the fix is in the code that is compiled in velero binary.
It was fixed due to the bump up introduced by #4022
More specifically https://github.com/vmware-tanzu/velero/pull/4022/files#diff-33ef32bf6c23acb95f5902d7097b7a1d5128ca061167ec0716715b0b9eeaa5f6L42-R41

So the version of k8s that velero server runs on should be irrelevant

@jenting
Copy link
Contributor

jenting commented Sep 1, 2021

The upstream fixes this issue at Kubernetes 1.20+?
If yes, will this change impact the user using the Kubernetes < 1.20 to do restore?

@jenting
I think the fix is in the code that is compiled in velero binary.
It was fixed due to the bump up introduced by #4022
More specifically https://github.com/vmware-tanzu/velero/pull/4022/files#diff-33ef32bf6c23acb95f5902d7097b7a1d5128ca061167ec0716715b0b9eeaa5f6L42-R41

So the version of k8s that velero server runs on should be irrelevant

Okay, it's like fixed in k8s.io/apimachinery in 0.20.x version.
Good to know, thank you.

Copy link
Contributor

@jenting jenting left a comment

Choose a reason for hiding this comment

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

Thank you for removing this workaround.

@jenting jenting merged commit 746cd61 into vmware-tanzu:main Sep 1, 2021
@sseago
Copy link
Collaborator

sseago commented Sep 1, 2021

@jenting Also, the associated test should fail if we ever pull in a version of kubernetes code without the fix -- the reproducing test was added when moving to the unstructured workaround.

danfengliu pushed a commit to danfengliu/velero that referenced this pull request Jan 25, 2022
This commit removes `IsUnstructuredCRDReady` since
kubernetes/kubernetes#87675 is fixed.
Is uses `Is1CRDReady` to check the readiness of CRD.

After v1.7 we may consider merge the funcx `IsV1Beta1CRDReady` and
`IsV1CRDReady`

Signed-off-by: Daniel Jiang <jiangd@vmware.com>
gyaozhou pushed a commit to gyaozhou/velero-read that referenced this pull request May 14, 2022
This commit removes `IsUnstructuredCRDReady` since
kubernetes/kubernetes#87675 is fixed.
Is uses `Is1CRDReady` to check the readiness of CRD.

After v1.7 we may consider merge the funcx `IsV1Beta1CRDReady` and
`IsV1CRDReady`

Signed-off-by: Daniel Jiang <jiangd@vmware.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has-unit-tests kind/changelog-not-required PR does not require a user changelog. Often for docs, website, or build changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

clean up IsUnstructuredCRDReady
4 participants