Promote ApigeeInstance to v1beta1 #3599
Conversation
|
/hold need to rebase after #3593 merges |
jasonvigil
force-pushed
the
apigee-instance-promote
branch
4 times, most recently
from
ff7f570 to
43a5bde
Compare
jasonvigil
force-pushed
the
apigee-instance-promote
branch
from
43a5bde to
14bdd1b
Compare
|
/hold cancel |
| @@ -24,7 +24,7 @@ var ApigeeEnvgroupGVK = GroupVersion.WithKind("ApigeeEnvgroup") | |||
|
|
|||
| type Parent struct { | |||
| // +required | |||
| OrganizationRef *refv1alpha1.ApigeeOrganizationRef `json:"organizationRef"` | |||
| OrganizationRef *refs.ApigeeOrganizationRef `json:"organizationRef"` | |||
There was a problem hiding this comment.
We should move the ApigeeOrganizationRef to apis/apigee? So version promotion do not need to change the import, and we can have different versions for ApigeeOrganizationRef
There was a problem hiding this comment.
Will do, in a follow-up PR.
| @@ -19,7 +19,7 @@ import ( | |||
| "fmt" | |||
| "strings" | |||
|
|
|||
| refs "github.com/GoogleCloudPlatform/k8s-config-connector/apis/refs/v1alpha1" | |||
| refs "github.com/GoogleCloudPlatform/k8s-config-connector/apis/refs/v1beta1" | |||
There was a problem hiding this comment.
Will do, in a follow-up PR.
| @@ -89,6 +89,7 @@ func addResourceConfig(t *testing.T, smLoader *servicemappingloader.ServiceMappi | |||
| func IsPureDirectResource(gk schema.GroupKind) bool { | |||
There was a problem hiding this comment.
@maqiuyujoyce Could you confirm if we still need this hack with the removal of the service mapping ?
There was a problem hiding this comment.
I created #3614 to track this improvement
There was a problem hiding this comment.
My change in #3443 didn't touch this hack so yes, let's still use it to avoid potential issues.
@jasonvigil filed an issue #3614 (thank you, Jason) and I'll take a look at it later.
| @@ -65,6 +65,7 @@ | |||
| [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 | |||
| [missing_field] crd=apigeeinstances.apigee.cnrm.cloud.google.com version=v1beta1: field ".spec.consumerAcceptList[]" is not set in unstructured objects | |||
There was a problem hiding this comment.
This will leave the field uncovered (and does not meet the exit criteria). Can we fix the code to make the CRD check pass? cc @acpana
There was a problem hiding this comment.
Update method misses 5 fields (are they all immutable? I don't see the immutable marker in the API). Something like Description, DisplayName, KMSKey seems to be mutable. Even if they are not, it could be a better UX to tell users why those fields are not updated by checking all the changed fields. Because users need to get this information to fix their config.
There was a problem hiding this comment.
In my experiments, the API does not support changing these fields. Are you suggesting we should allow user to attempt to change them anyway, and return the error from GCP?
There was a problem hiding this comment.
Sorry, I didn't make it clear. We can either "allow user to attempt to change them anyway, and return the error from GCP" or add the check in the controller. The advantage for the former is that the users can get the exact error message from GCP and once GCP changes the behavior KCC does not need to update the code. The disadvantage is repeated reconcile failures (which may not be too bad).
There was a problem hiding this comment.
Sounds good, I agree. Will make the change
|
/approve A few nits for better UX and maintenance. |
|
[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 |
jasonvigil
force-pushed
the
apigee-instance-promote
branch
from
14bdd1b to
dccaa03
Compare
|
/lgtm |
google-oss-prow
bot
merged commit 0a48b49
into
GoogleCloudPlatform:master
| @@ -12,14 +12,13 @@ | |||
| // See the License for the specific language governing permissions and | |||
| // limitations under the License. | |||
|
|
|||
| package v1alpha1 | |||
| package v1beta1 | |||
There was a problem hiding this comment.
Ah I think this bit me in #3605
No description provided.