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

HTTP ingress rule value is Mandatory #402

Merged
merged 1 commit into from
Mar 29, 2021
Merged

Conversation

kuritka
Copy link
Collaborator

@kuritka kuritka commented Mar 22, 2021

related to #401

  • the custom structures with mandatory field overrides IngressSpec and
    and implements necessary routines like DeepCopyInto and transformations between
    k8gbv1beta1 and v1beta1

  • controller-gen created DeepCopy() functions in zz_generated.deepcopy.go

@kuritka kuritka force-pushed the ingress-mandatory-http branch from 84dd9e8 to 8aedea5 Compare March 22, 2021 12:59
@kuritka kuritka requested a review from k0da March 22, 2021 13:00
@kuritka
Copy link
Collaborator Author

kuritka commented Mar 22, 2021

@k0da amended

Copy link
Member

@ytsarev ytsarev left a comment

Choose a reason for hiding this comment

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

From https://kubernetes.io/blog/2019/07/18/api-deprecations-in-1-16/

The v1.22 release will stop serving the following deprecated API versions in favor of newer and more stable API versions:

Ingress in the extensions/v1beta1 API version will no longer be served
Migrate to use the networking.k8s.io/v1beta1 API version, available since v1.14. Existing persisted data can be retrieved/updated via the new version.

So we should stick with networking.k8s.io/v1beta1

@kuritka
Copy link
Collaborator Author

kuritka commented Mar 22, 2021

@ytsarev, @k0da - so should we upgrade to networking.k8s.io/v1beta1 first and then migrate this PR to networking.k8s.io/v1beta1 ??

@k0da
Copy link
Collaborator

k0da commented Mar 22, 2021

@kuritka IMO we can do both

@kuritka kuritka force-pushed the ingress-mandatory-http branch 4 times, most recently from ccb22d0 to 6983551 Compare March 29, 2021 08:55
@kuritka
Copy link
Collaborator Author

kuritka commented Mar 29, 2021

@ytsarev @k0da @somaritane , I succesfully migrated to networking.k8s.io/v1beta1 (see: this), but to make PR slim I rather split large one into into two smaller PR's.

This one is making HTTPIngressRuleValue mandatory. The migration would go to the second PR.

just in case: In second PR I will change CRD.

k0da
k0da previously approved these changes Mar 29, 2021
Copy link
Collaborator

@k0da k0da left a comment

Choose a reason for hiding this comment

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

👍

@kuritka kuritka requested a review from ytsarev March 29, 2021 10:38
Copy link
Member

@ytsarev ytsarev left a comment

Choose a reason for hiding this comment

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

some cosmetic comments with the focus on future maintenance

@@ -0,0 +1,196 @@
/*
Copy link
Member

Choose a reason for hiding this comment

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

What is the filename semantics? types vs gslb_types?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ytsarev good question,
It lives originally in k8s.io/api/extensions/v1beta1/types.go.

Copy link
Member

Choose a reason for hiding this comment

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

upstream_types.go maybe?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Renamed, was just following original convention upstream_types.go is well descriptive.

"k8s.io/api/extensions/v1beta1"
)

type IngressSpec struct {
Copy link
Member

Choose a reason for hiding this comment

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

Can we reference the upstream source of IngressSpec for future?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ytsarev , good idea, amended

@kuritka kuritka force-pushed the ingress-mandatory-http branch 3 times, most recently from e4b9c92 to affa432 Compare March 29, 2021 12:52
@kuritka kuritka requested review from somaritane, k0da and ytsarev March 29, 2021 12:53
Copy link
Member

@ytsarev ytsarev left a comment

Choose a reason for hiding this comment

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

IngressSpec Wording suggestion.

- the custom structures with mandatory field overrides IngressSpec and
and implements necessary routines like DeepCopy and transformations between
`k8gbv1beta1` and `v1beta1`

- controller-gen created `DeepCopy()` functions in `zz_generated.deepcopy.go`

Co-authored-by: Yury Tsarev <[email protected]>
@kuritka kuritka force-pushed the ingress-mandatory-http branch from 1c67553 to e62e0ed Compare March 29, 2021 17:24
@kuritka kuritka requested review from somaritane and ytsarev March 29, 2021 17:37
Copy link
Contributor

@somaritane somaritane left a comment

Choose a reason for hiding this comment

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

lgtm

@kuritka kuritka merged commit d30d186 into master Mar 29, 2021
@kuritka kuritka deleted the ingress-mandatory-http branch March 30, 2021 08:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants