-
Notifications
You must be signed in to change notification settings - Fork 65
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
Rename serialized ForProvider as "forProvider" #309
Conversation
Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>
@@ -230,12 +230,11 @@ type SubnetList struct { | |||
// A PublicIPAddressSpec defines the desired state of a PublicIPAddress. | |||
type PublicIPAddressSpec struct { | |||
xpv1.ResourceSpec `json:",inline"` | |||
// +kubebuilder:validation:Required | |||
ForProvider PublicIPAddressFormat `json:"properties"` | |||
ForProvider PublicIPAddressProperties `json:"forProvider"` |
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.
Per Crossplane conventions, we had better rename the serialized form as forProvider
instead of properties
.
// PublicIPAddressFormat defines properties of the PublicIPAddress. | ||
type PublicIPAddressFormat struct { | ||
// PublicIPAddressProperties defines properties of the PublicIPAddress. | ||
type PublicIPAddressProperties struct { |
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.
Again let's stick to the repo conventions by using Properties
as a suffix.
// +kubebuilder:validation:Enum=Standard;Basic | ||
Name string `json:"name,omitempty"` | ||
Name string `json:"name"` |
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.
spec.forProvider.sku
is optional. However, if a spec.forProvider.sku
is specified, I think we need to have its name also specified. Thus removing the omitempty
tag.
Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>
Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>
- Previously, these were IPV4 & IPv6, which do not match Azure SDK constants Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>
bcfb32e
to
072fe18
Compare
// A PublicIPAddressStatus represents the observed state of a SQLServer. | ||
type PublicIPAddressStatus struct { | ||
xpv1.ResourceStatus `json:",inline"` | ||
AtProvider PublicIPAddressObservation `json:"atProvider,omitempty"` |
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.
Per Crossplane conventions, let's move the observation under to status.atProvider
.
PublicIPAllocationMethod string `json:"allocationMethod"` | ||
|
||
// PublicIPAllocationMethod - The public IP address version. Possible values include: 'IPV4', 'IPV6' | ||
// PublicIPAllocationMethod - The public IP address version. Possible values include: 'IPv4', 'IPv6' |
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.
Looks like the SDK constants are defined as IPv4
and IPv6
.
072fe18
to
1abc62f
Compare
} | ||
|
||
// PublicIPAddressFormat defines properties of the PublicIPAddress. | ||
type PublicIPAddressFormat struct { | ||
// PublicIPAddressProperties defines properties of the PublicIPAddress. |
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.
FYI, looks like there are many more properties that can be specified for PublicIPAdress
.
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 may consider adding them in later PRs unless there is immediate need, what do you think @muvaf?
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.
I personally prefer making it complete while I have the context of the resource, otherwise I'd have to load all that later again, but it can be merged as an alpha resource with less properties than full.
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.
It turns out that we are not sure regarding the immediate requirements around this resource. Hence, I've extended the API surface.
1abc62f
to
bb4cb3d
Compare
- spec.forProvider.{version,allocationMethod,location} are now marked as immutable and are not considered during drift detection. SDK does not allow such updates in a straightforward manner. Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>
8d4ef4a
to
759d10e
Compare
Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>
759d10e
to
2a6da58
Compare
- Remove "FQDN" from spec.atProvider's DNS settings as it's a read-only property Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>
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.
Thanks @ulucinar ! Looks good to merge after resolving non-nitpick comments.
if err := e.kube.Update(ctx, s); err != nil { | ||
return managed.ExternalObservation{}, errors.Wrap(err, errUpdateCR) | ||
} | ||
|
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 usually update only if there is a diff and used to do kube.Update
for late-init in old controllers but the recommended way today is to return ResourceLateInitialized: true
to make managed reconciler handle it.
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.
Thanks @muvaf for the ResourceLateInitialized: true
suggestion. I've removed the embedded controller-runtime client from our external client implementation and now always set ObservationResult.ResourceLateInitialized
accordingly.
We can at a later time revisit provider-azure
's late-initialization utility functions so that they report whether fields have been late-initialized.
While working on this, I had also considered using cmp.Equal
to detect any changes after late-initialization, however, I did not choose to do so as API server already detects noops and does not update the resource. However, leaving update calls to the managed reconciler is I think a good compromise.
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.
I just realized that if we leave this to the managed reconciler, we are losing any status updates. So, I'm reverting back to making the update call in-site as we already do elsewhere in the repo:
https://github.com/crossplane/provider-azure/blob/8380dd2d85e0391bd02e23c5c47c13f8962c38a7/pkg/controller/database/postgresqlserver/managed.go#L122
So returning ResourceLateInitialized: true
makes us lose status updates and I'd like to address proper calculation of this in a repo-wide manner.
As I mentioned, one alternative could be to:
- Make a deep-copy of the
spec.forProvider
- Run late-initialization logic on it
- Compare it with the current
spec.forProvider
usingcmp.Equal
to determine whether there is a spec change,
but I prefer not to take this approach right now as I don't believe it's necessary to do so. We could modify the late-init utilities in the repo to report such updates.
Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>
1c74f51
to
3ff1b85
Compare
Description of your changes
I have:
make reviewable test
to ensure this PR is ready for review.How has this code been tested
Manually tested using the example manifest provided in the repo,
examples/network/publicipaddress.yaml
.