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

ConstraintTemplate versions use apiextensionsv1 #123

Conversation

julianKatz
Copy link
Contributor

Previously, I left ConstraintTemplate (CT) versions v1alpha1 and v1beta1
using apiextensionsv1beta1.JSONSchemaProps. I thought this appropriate,
given that was what they were previously implemented as. v1 CT, on the
other hand, would use v1beta1 JSONSchemaProps.

This turns out to be a bad idea.

Any logic we write that interacts with OpenAPIV3Schema in a CT's
Validation section is forced to interact with OpenAPIV3Schema's type.
Before this PR, that type is apiextensionsv1beta1.JSONSchemaProps. This
makes our life difficult, as v1 CT's version of this variable will have
a different type. This would then require us to re-implement any logic
for this section to implement a second type, or to do some gross
conversions.

Further, apiextensionsv1beta1 will be removed from
k8s.io/apiextensions-apiserver, preventing us from upgrading that
library without doing this work.

Basically, this change is unavoidable and appropriate.

This blocks #121

Contributes to open-policy-agent/gatekeeper#550

Signed-off-by: juliankatz juliankatz@google.com

Previously, I left ConstraintTemplate (CT) versions v1alpha1 and v1beta1
using apiextensionsv1beta1.JSONSchemaProps.  I thought this appropriate,
given that was what they were previously implemented as.  v1 CT, on the
other hand, would use v1beta1 JSONSchemaProps.

This turns out to be a bad idea.

Any logic we write that interacts with OpenAPIV3Schema in a CT's
Validation section is forced to interact with OpenAPIV3Schema's type.
Before this PR, that type is apiextensionsv1beta1.JSONSchemaProps.  This
makes our life difficult, as v1 CT's version of this variable will have
a different type.  This would then require us to re-implement any logic
for this section to implement a second type, or to do some gross
conversions.

Further, apiextensionsv1beta1 will be removed from
k8s.io/apiextensions-apiserver, preventing us from upgrading that
library without doing this work.

Basically, this change is unavoidable and appropriate.

This blocks open-policy-agent#121

Contributes to open-policy-agent/gatekeeper#550

Signed-off-by: juliankatz <juliankatz@google.com>
@julianKatz julianKatz force-pushed the transition-to-apiextensionsv1-jsonshemaprops branch from 88ba532 to 04f102c Compare June 9, 2021 00:51
@codecov-commenter
Copy link

codecov-commenter commented Jun 9, 2021

Codecov Report

Merging #123 (04f102c) into master (5c03494) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #123   +/-   ##
=======================================
  Coverage   44.26%   44.26%           
=======================================
  Files          30       30           
  Lines        2417     2417           
=======================================
  Hits         1070     1070           
  Misses       1022     1022           
  Partials      325      325           
Flag Coverage Δ
unittests 44.26% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5c03494...04f102c. Read the comment docs.

Copy link
Contributor

@maxsmythe maxsmythe left a comment

Choose a reason for hiding this comment

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

you ran make generate and there were no manifest changes?

If so, LGTM

@shomron @ritazh @sozercan LGTY?

@julianKatz
Copy link
Contributor Author

you ran make generate and there were no manifest changes?

make manifests returns no manifest changes 👍🏼

make generate does code gen :)

@ritazh
Copy link
Member

ritazh commented Jun 9, 2021

make manifests generated diff in the following files but they are all line break updates.
modified: config/crds/templates.gatekeeper.sh_constrainttemplates.yaml
modified: deploy/crds.yaml

@maxsmythe maxsmythe merged commit 878a63a into open-policy-agent:master Jun 9, 2021
@julianKatz
Copy link
Contributor Author

make manifests generated diff in the following files but they are all line break updates.
modified: config/crds/templates.gatekeeper.sh_constrainttemplates.yaml
modified: deploy/crds.yaml

@ritazh What version of controller-gen are you running? I just tried again and didn't see any diff. Mine is:

❯ controller-gen --version
Version: v0.5.0

@julianKatz julianKatz deleted the transition-to-apiextensionsv1-jsonshemaprops branch June 9, 2021 23:22
@ritazh
Copy link
Member

ritazh commented Jun 9, 2021

@julianKatz it's showing Version: v0.5.0

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.

None yet

4 participants