fix: comment status without SSA options

[fernandezcuesta]

Description of your changes

Remove SSA markers for listType: map and injected key defaults in the observation (status) fields.

Fixes #659

I have:

• Read and followed Upjet's contribution process.
• Run make reviewable to ensure this PR is ready for review.
  - [ ] Added backport release-x.y labels 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.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 23177b3e-4ae2-4fe0-ad78-13f3d4e33158

📥 Commits

Reviewing files that changed from the base of the PR and between ab443d2 and 917cb8c.

📒 Files selected for processing (2)

• pkg/types/builder_test.go
• pkg/types/field.go

🚧 Files skipped from review as they are similar to previous changes (2)

• pkg/types/field.go
• pkg/types/builder_test.go

---

📝 Walkthrough

Walkthrough

The PR removes SSA list-map markers and injected-key defaults from Observation comment generation, and adds test coverage to verify the generated comment markers are present on Parameters but absent on Observation.

Changes

SSA Marker Removal and Test Coverage

Layer / File(s)	Summary
Strip SSA markers from Observation fields pkg/types/field.go	Before adding a field to Observation, the code clears comment reference metadata, removes ListTypeMap merge markers, and unsets any injected-key default.
Validate SSA marker isolation in test coverage pkg/types/builder_test.go	TestBuild adds optional comment checks, and a new SSA case validates marker placement and default removal in generated comments.

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested labels

backport release-1.11, backport release-1.10

Suggested reviewers

• sergenyalcin
• erhancagirici
• ulucinar

🚥 Pre-merge checks | ✅ 7

✅ Passed checks (7 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title is concise and accurately describes removing SSA options from status comments.
Description check	✅ Passed	The description clearly matches the fix to remove SSA markers from observation fields.
Linked Issues check	✅ Passed	The code and test address #659 by stripping SSA markers and injected defaults from observation comments.
Out of Scope Changes check	✅ Passed	The diff stays focused on the observation-comment fix and the supporting test.
Configuration Api Breaking Changes	✅ Passed	PASS: the PR delta touches only pkg/types; git diff-tree shows no pkg/config/** changes, so no exported config API was removed, renamed, or re-signed.
Generated Code Manual Edits	✅ Passed	Only pkg/types/builder_test.go and pkg/types/field.go changed; no files matching zz_*.go were touched.
Template Breaking Changes	✅ Passed	Diff only changes pkg/types/field.go and builder_test.go; no pkg/controller/external*.go templates were modified, so this check is not triggered.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

pkg/types/builder_test.go (1)

642-642: ⚡ Quick win

Please 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

📥 Commits

Reviewing files that changed from the base of the PR and between 0beea8e and a7c0393.

📒 Files selected for processing (2)

• pkg/types/builder_test.go
• pkg/types/field.go

[Upbound-CLA]

All committers have signed the CLA.

[Upbound-CLA]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[jonasz-lasut]

Hi @fernandezcuesta I've tested your upjet fork against provider-upjet-azure and noticed a side affect which was most likely not planned - x-kubernetes-list-type: map and injected key defaults were removed from the observation which is a viable solution to issues related to observations defaulting injected key multiple times but x-kubernetes-list-type: set and x-kubernetes-map-type: granular markers were also removed which introduces possible regressions and does not solve any existing issue like the one related to injected keys.

Would you be open to implementing a filter so that only the fields related to the injected key behavior are removed from observation? So default: nil + removal of x-kubernetes-list-type: map as it requires now non-existing required/defaulting field

[fernandezcuesta]

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 .status field. See https://github.com/crossplane/crossplane-runtime/blob/main/pkg/reconciler/managed/reconciler.go#L1005 where calling Status().Update() effectively does a classic PUT in /status, not an SSA patch operation.

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).

[jonasz-lasut]

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 .status reasoning

[fernandezcuesta]

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.

[jonasz-lasut]

Thank you for pushing the change, I'll try to test it with Azure ApplicationGateway today or tomorrow

[jonasz-lasut]

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

[jonasz-lasut]

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