-
Notifications
You must be signed in to change notification settings - Fork 245
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
feat: Promote Apigee Envgroup to v1beta1 #3544
feat: Promote Apigee Envgroup to v1beta1 #3544
Conversation
config/servicemappings/apigee.yaml
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maqiuyujoyce Eventually we want to drop this step about configuring the service mapping for direct resource. How does your PR go?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah we will need to rebase or fix up the code after #3443 merges
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kk. I saw the full test suite has coverage for this. And I like the asSortedCopy
comparison. Good job!
3ad05f9
to
4c5cba3
Compare
/assign @jasonvigil |
65cfc3e
to
4e304b9
Compare
4e304b9
to
4335186
Compare
pkg/controller/direct/apigee/testdata/fuzz/FuzzApigeeEnvgroupObservedState/00d24b398845eea4
Outdated
Show resolved
Hide resolved
pkg/controller/direct/apigee/testdata/fuzz/FuzzApigeeEnvgroupSpec/00d24b398845eea4
Outdated
Show resolved
Hide resolved
4335186
to
4f0010d
Compare
b9fcc46
to
807f4a2
Compare
/retest |
807f4a2
to
9548a84
Compare
/lgtm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also update the fixture test create.yaml and update.yaml to use the Beta APIs?
@@ -64,6 +64,7 @@ | |||
[missing_field] crd=alloydbinstances.alloydb.cnrm.cloud.google.com version=v1beta1: field ".spec.gceZone" is not set in unstructured objects | |||
[missing_field] crd=alloydbinstances.alloydb.cnrm.cloud.google.com version=v1beta1: field ".spec.networkConfig.authorizedExternalNetworks[].cidrRange" is not set in unstructured objects | |||
[missing_field] crd=alloydbusers.alloydb.cnrm.cloud.google.com version=v1beta1: field ".spec.databaseRoles[]" is not set in unstructured objects | |||
[missing_field] crd=apigeeenvgroups.apigee.cnrm.cloud.google.com version=v1beta1: field ".spec.hostnames[]" is not set in unstructured objects |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need full field coverage before promoting to Beta.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created #3571 to track this issue.
dev/tasks/find-missing-fields
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We no longer need to update this file. Here's an example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For Apigee, we need to write custom fuzz tests because there is no proto-based API. The new fuzz framework requires proto-based mappers, so we (unfortunately) cannot use it. Similarly, I don't think this should be a blocker. CC @acpana
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: yuwenma The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/lgtm |
93b3f98
into
GoogleCloudPlatform:master
Change description
Fixes #
Tests you have done
make ready-pr
to ensure this PR is ready for review.