fix: comment status without SSA options#660
Conversation
Signed-off-by: Jesús Fernández <7312236+fernandezcuesta@users.noreply.github.com>
fernandezcuesta
requested review from
erhancagirici,
sergenyalcin and
ulucinar
as code owners
fernandezcuesta
changed the title
📝 WalkthroughWalkthroughThe PR fixes a bug where server-side apply merge-strategy markers were incorrectly applied to Observation struct fields. When status lists contained multiple items, the API server rejected updates due to conflicting default values. The fix strips SSA markers and defaults from Observation fields, with test coverage validating that markers appear only in the Spec side (Parameters) but not in the Status side (Observation). ChangesSSA Marker Removal and Test Coverage
🎯 2 (Simple) | ⏱️ ~10 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (6 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/types/builder_test.go (1)
642-642: ⚡ Quick winPlease rename the test case key to PascalCase (no underscores).
Nice coverage addition here. Could you rename
"SSA_InjectedKey_Not_In_Observation_Comments"to a PascalCase form (for example,SSAInjectedKeyNotInObservationComments) to match repo test naming conventions? Thanks for adding this regression case.As per coding guidelines,
**/*_test.go: “Enforce table-driven test structure: PascalCase test names (no underscores)”.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/types/builder_test.go` at line 642, Rename the table-driven test case key "SSA_InjectedKey_Not_In_Observation_Comments" to a PascalCase identifier (no underscores), e.g., SSAInjectedKeyNotInObservationComments, so it matches the repository's test naming convention; update any places that reference this map key inside builder_test.go (the test cases map entry and any lookups/assertions) to use the new PascalCase name.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/types/builder_test.go`:
- Line 642: Rename the table-driven test case key
"SSA_InjectedKey_Not_In_Observation_Comments" to a PascalCase identifier (no
underscores), e.g., SSAInjectedKeyNotInObservationComments, so it matches the
repository's test naming convention; update any places that reference this map
key inside builder_test.go (the test cases map entry and any lookups/assertions)
to use the new PascalCase name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 89a04c5c-8ca1-44c4-ab0b-02e8246b30d5
📒 Files selected for processing (2)
pkg/types/builder_test.gopkg/types/field.go
|
|
|
|
|
Hi @fernandezcuesta I've tested your upjet fork against provider-upjet-azure and noticed a side affect which was most likely not planned - Would you be open to implementing a filter so that only the fields related to the injected key behavior are removed from observation? So |
|
Hi @jonasz-lasut, I thought a bit about that when first implementing this PR, then decided to be pragmatic and remove all observation markers. The reasoning supporting it is that we don't do SSA patch operations (honoring SSA merge markers) on Having said that, if you still prefer to go fine-grained and only remove those markers, I'm completely fine too (we would avoid unnecessary CRD changes). |
|
Thank you for having a look @fernandezcuesta . My main goal was to avoid unnecessary CRD changes as it would lead to 1000+ file diffs in major cloud providers without any visible benefit at this moment. However I completely agree with the PUT on |
|
OK let's be minimally intrusive by now and do a surgical removal as you suggest, idk if at some point the behavior will change from crossplane-runtime, but going too broad would indeed break it. I'll amend it during the day. |
Signed-off-by: Jesús Fernández <7312236+fernandezcuesta@users.noreply.github.com>
|
Thank you for pushing the change, I'll try to test it with Azure ApplicationGateway today or tomorrow |
jonasz-lasut
left a comment
jonasz-lasut
left a comment
There was a problem hiding this comment.
Tested it with provider-upjet-azure and ApplicationGateway which ahd a long standing bug (linked to this issue already).
Following resources were used for the test, first with provider-azure in version 2.6.0 where Observe was failing and with provider built based on this upjet diff.
# SPDX-FileCopyrightText: 2024 The Crossplane Authors <https://crossplane.io>
#
# SPDX-License-Identifier: Apache-2.0
apiVersion: network.azure.m.upbound.io/v1beta1
kind: ApplicationGateway
metadata:
annotations:
meta.upbound.io/example-id: network/v1beta2/applicationgateway
uptest.upbound.io/timeout: "7200"
labels:
testing.upbound.io/example-name: network
name: example
namespace: upbound-system
spec:
forProvider:
backendAddressPool:
- name: example
backendHttpSettings:
- cookieBasedAffinity: Disabled
name: example
path: /path1/
port: 80
protocol: Http
requestTimeout: 60
frontendIpConfiguration:
- name: example
index: index1
publicIpAddressIdSelector:
matchLabels:
testing.upbound.io/example-name: example
- name: example2
index: index2
publicIpAddressIdSelector:
matchLabels:
testing.upbound.io/example-name: example2
frontendPort:
- name: example
port: 80
gatewayIpConfiguration:
- name: my-gateway-ip-configuration
subnetIdSelector:
matchLabels:
testing.upbound.io/example-name: frontend
httpListener:
- frontendIpConfigurationName: example
frontendPortName: example
name: example
protocol: Http
location: West Europe
requestRoutingRule:
- backendAddressPoolName: example
backendHttpSettingsName: example
httpListenerName: example
name: example
priority: 9
ruleType: Basic
resourceGroupNameSelector:
matchLabels:
testing.upbound.io/example-name: example
sku:
capacity: 2
name: Standard_v2
tier: Standard_v2
---
apiVersion: network.azure.m.upbound.io/v1beta1
kind: PublicIP
metadata:
annotations:
meta.upbound.io/example-id: network/v1beta2/applicationgateway
labels:
testing.upbound.io/example-name: example
name: example
namespace: upbound-system
spec:
forProvider:
allocationMethod: Static
location: West Europe
resourceGroupNameSelector:
matchLabels:
testing.upbound.io/example-name: example
sku: Standard
---
apiVersion: network.azure.m.upbound.io/v1beta1
kind: PublicIP
metadata:
annotations:
meta.upbound.io/example-id: network/v1beta2/applicationgateway
labels:
testing.upbound.io/example-name: example2
name: example2
namespace: upbound-system
spec:
forProvider:
allocationMethod: Static
location: West Europe
resourceGroupNameSelector:
matchLabels:
testing.upbound.io/example-name: example
ipVersion: IPv6
sku: Standard
---
apiVersion: network.azure.m.upbound.io/v1beta1
kind: Subnet
metadata:
annotations:
meta.upbound.io/example-id: network/v1beta2/applicationgateway
labels:
testing.upbound.io/example-name: frontend
name: frontend
namespace: upbound-system
spec:
forProvider:
addressPrefixes:
- 10.254.0.0/24
- 2001:db8:1234:5601::/64
resourceGroupNameSelector:
matchLabels:
testing.upbound.io/example-name: example
virtualNetworkNameSelector:
matchLabels:
testing.upbound.io/example-name: example
---
apiVersion: network.azure.m.upbound.io/v1beta1
kind: VirtualNetwork
metadata:
annotations:
meta.upbound.io/example-id: network/v1beta2/applicationgateway
labels:
testing.upbound.io/example-name: example
name: example
namespace: upbound-system
spec:
forProvider:
addressSpace:
- 10.254.0.0/16
- 2001:db8:1234:5601::/64
location: West Europe
resourceGroupNameSelector:
matchLabels:
testing.upbound.io/example-name: example
---
apiVersion: azure.m.upbound.io/v1beta1
kind: ResourceGroup
metadata:
annotations:
meta.upbound.io/example-id: network/v1beta1/applicationgateway
labels:
testing.upbound.io/example-name: example
name: example
namespace: upbound-system
spec:
forProvider:
location: West Europe
|
Thank you for your contribution @fernandezcuesta , I'll discuss this PR with the rest of maintainers on Monday but from my perspective it's ok to merge |
3 participants
Description of your changes
Remove SSA markers for
listType: mapand injected key defaults in the observation (status) fields.Fixes #659
I have:
make reviewableto ensure this PR is ready for review.- [ ] Addedbackport release-x.ylabels to auto-backport this PR if necessary.How has this code been tested
Added a test.
Real e2e tested with provider-pageduty in
fernandezcuesta/provider-pagerduty:fix-ssa-markers.