ai: update skills, chores#9683
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
ldanielmadariaga
left a comment
ldanielmadariaga
left a comment
There was a problem hiding this comment.
Let's add some comments about copyright years as well :) I keep having to reject PRs because the agent will update copyright years for existing files
| - Ensure GCP acronyms are capitalized correctly in resource and field names (e.g., `APIHub`, `SIP`, `JSON`, `PDF`, `DER`, `CEL`). | ||
| - **No Exceptions**: Do **NOT** add new exceptions to `tests/apichecks/testdata/exceptions/acronyms.txt`. Fix the Go field names instead. | ||
| - **Pointers**: Validate that optional or nullable fields (like `Location`, booleans, integers) are correctly typed as pointers (e.g., `*string`, `*bool`) rather than raw values. | ||
| - **ETag Exclusion**: Remove `etag` fields from the KRM spec; they are generally disallowed. Do **NOT** add exceptions to `spec_dislike_etag.txt`. |
There was a problem hiding this comment.
Sorry to pile on here but we just don't support etags? Is this based on some team decision made in the past? In principle I get with supporting etags is messy in KRM mostly trying to get context.
| - **KCC References (`missingrefs.txt`)**: | ||
| - Ensure fields representing GCP resources are implemented using the KCC Ref pattern (e.g., `ProjectRef`, `NetworkRef`) rather than plain strings. | ||
| - **No Exceptions**: Do **NOT** add exceptions to `tests/apichecks/testdata/exceptions/missingrefs.txt`. Fix the underlying field types instead. | ||
| - Reuse existing refs from `apis/refs/v1beta1/` (e.g., `Compute`, `BigQueryDataset`). If creating a new ref, put it in a service-specific file (e.g., `apis/dlp/v1alpha1/dlprefs.go`). |
There was a problem hiding this comment.
I've seen inconsistent use and recommendations here. Most of the resources are doing the contrary to this :)
Why are we settling on this pattern now? I also see that some of the refs in api/refs don't really follow the new standards. I think these mostly reside in apis/refs rather than apis/refs/v1beta1 we should remove these to avoid confusion.
There was a problem hiding this comment.
Why are we settling on this pattern now?
IIUC, we are marching towards having every ref in its api package, i.e. apis/dlp/v1alpha1/resource_reference.go; and we wanted to move away from apis/refs/v1beta1 as they continued old style refs but @gemmahou has more context here.
There was a problem hiding this comment.
That's what I understood from talking to you. I fear that 'Reuse existing refs from apis/refs/v1beta1/' will lead the bot to prefer that folder rather than api packages may be we should scope it down to just very speicific refs like: project, org, folder?
Anyways I'd reword the text a bit to block possible generalizations assumptions like:
When creating a new ref you must put it in a service-specific file (e.g., apis/dlp/v1alpha1/dlp_reference.go). You can reuse some already existing refs from apis/refs/v1beta1/ (e.g., Compute, BigQueryDataset). You must only reuse refs from the apis/refs/v1beta1/ you must not use refs under apis/refs/ as these use a deprecated style that is no longer correct. You must not create new reference files under apis/refs or any subfolder within.
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
acpana
force-pushed
the
acpana/chore-updates
branch
from
3e6cf86 to
3f61235
Compare
2 participants
Per my greatest hits (aka most frequent comments) when reviewing PRs, this is what the bots think it will be useful