Use ID fields for internal referencing in local source #211

meyskens · 2025-04-24T12:50:30Z

Builds on #208 and changes provisioning in local data source to use the IDs instead of the names.

Fixes #204 and #205

meyskens · 2025-04-25T14:19:36Z

To make the tests functional again this PR has gone way beyond simple just replace the IDS.

This PR now also covers #203 and outputs the IDs as well as accept them as input.

markgoddard · 2025-05-09T15:57:09Z

Merged in main, resolved conflicts.

meyskens · 2025-05-14T13:44:10Z

Used a merge as a rebase caused 30x more conflitcs, before we merge this into main we should solidify the commits

Copilot

Pull Request Overview

This pull request migrates local source provisioning and configuration to reference trust zones, clusters, attestation policies, and AP bindings via their unique ID fields instead of names. Key changes include modifying internal functions and data fixtures, updating CUE and YAML schemas, and adjusting CLI commands and tests to use the new ID fields.

Reviewed Changes

Copilot reviewed 38 out of 38 changed files in this pull request and generated 2 comments.

File	Description
internal/pkg/*	Updated internal functions to use ID-based lookups
internal/pkg/config/*	Modified fixtures, tests, and schema to support trust zone and cluster IDs
cmd/cofidectl/cmd/*	Updated CLI commands and flags to handle IDs for workload, trust zone, cluster, federation, attestation policy, and AP bindings
go.mod	Upgraded dependency versions

Comments suppressed due to low confidence (1)

cmd/cofidectl/cmd/trustzone/helm/helm.go:92

The FIXME comment indicates an inconsistency where overrideValues receives a trust zone ID but may require the trust zone name. Consider aligning the API—either update the underlying provisioning to accept an ID or pass the expected name—and remove the FIXME comment once resolved.

// FIXME: pass the trust zone name instead of the ID.

cmd/cofidectl/cmd/workload/workload.go

cmd/cofidectl/cmd/cluster/cluster.go

Copilot

Pull Request Overview

This PR migrates internal referencing from names to unique ID fields across the codebase, updating data source filters, fixtures, configuration schemas, and CLI commands/tests.

Switch data source calls (e.g., ListClusters, ListFederations) to use ID-based filters
Add Id properties to fixtures, schema, and test data; introduce GetByID helpers
Extend CLI commands and flags to accept *-id parameters alongside or in place of names

Reviewed Changes

Copilot reviewed 38 out of 38 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
internal/pkg/trustzone/trustzone.go	Switch ListClusters to use `TrustZoneId` filter
internal/pkg/test/fixtures/fixtures.go	Add `Id` fields to trust zone, federation, cluster, and policy fixtures
internal/pkg/config/testdata/config/missing_trust_zone_field.yaml	Update federations to use `trust_zone_id` and `remote_trust_zone_id`
internal/pkg/config/testdata/config/full.yaml	Add ID attributes for trust zones, clusters, and policies
internal/pkg/config/schema.cue	Modify schema to require `trust_zone_id`, `policy_id`, etc.
internal/pkg/config/config_test.go	Update tests to use ID-based trustZone values
internal/pkg/config/config.go	Introduce `Get*ByID` methods and switch lookups to ID fields
internal/pkg/attestationpolicy/attestationpolicy.go	Change cluster listing and trust zone retrieval to use IDs
go.mod	Bump `cofide-api-sdk` to v0.20.0 and adjust dependencies
cmd/cofidectl/cmd/workload/workload.go	Add `--trust-zone-id` flag; update List/Status/Discover logic
cmd/cofidectl/cmd/trustzone/trustzone_test.go	Adapt tests for ID-based trust zone and cluster operations
cmd/cofidectl/cmd/trustzone/trustzone.go	Refactor add/del/status commands to use ID flags
cmd/cofidectl/cmd/trustzone/helm/helm.go	Support ID-based lookup in `override` and `values` commands
cmd/cofidectl/cmd/statusspinner/statusspinner.go	Update import path to `provision_plugin/v1alpha2`
cmd/cofidectl/cmd/federation/federation.go	Migrate federation commands to ID-based filters and flags
cmd/cofidectl/cmd/down.go	Support `--trust-zone-id` flag for teardown
cmd/cofidectl/cmd/cluster/cluster.go	Update ListClusters and deletion to use `trust_zone_id`
cmd/cofidectl/cmd/attestationpolicy/attestationpolicy.go	Add ID-based delete operation for attestation policies
cmd/cofidectl/cmd/apbinding/apbinding.go	Switch APBinding list/add/del to use ID-based flags

Comments suppressed due to low confidence (1)

cmd/cofidectl/cmd/trustzone/trustzone.go:279

DataSource interface provides GetTrustZone(id string) rather than GetTrustZoneByName; this call will not compile. Use ds.GetTrustZone(opts.name).

tz, err := ds.GetTrustZoneByName(opts.name)

cmd/cofidectl/cmd/workload/workload.go

cmd/cofidectl/cmd/federation/federation.go

Copilot

Pull Request Overview

This PR updates internal data provisioning to reference trust zones, clusters, attestation policies, and federations by their IDs rather than their names. Key changes include modifications to API calls in data source functions, updates to fixture and test definitions, and revisions to CLI commands so that both names and IDs can be used (with a focus on IDs).

Reviewed Changes

Copilot reviewed 38 out of 38 changed files in this pull request and generated 1 comment.

File	Description
internal/pkg/trustzone/trustzone.go	Replaces name-based cluster lookup with ID-based filtering
internal/pkg/test/fixtures/fixtures.go	Updates fixture definitions to include new ID fields for trust zones and clusters
internal/pkg/config/*	Schema and configuration tests are updated to expect ID fields rather than names
cmd/cofidectl/*	Updates CLI flags and command logic across workload, trustzone, cluster, and federation commands for ID-based referencing

Comments suppressed due to low confidence (2)

cmd/cofidectl/cmd/apbinding/apbinding.go:231

Both the name and ID for the trust zone (and similarly for attestation policy) are bound to the same variable, which can create ambiguity in flag parsing. It is recommended to use separate variables to distinctly capture the trust zone name and trust zone ID (and similarly for attestation policy) to avoid potential misconfiguration.

f.StringVar(&opts.trustZone, "trust-zone", "", "Trust zone name")
	f.StringVar(&opts.trustZone, "trust-zone-id", "", "Trust zone ID")

cmd/cofidectl/cmd/down.go:81

[nitpick] The flag name 'trustzone-id' is inconsistent with the naming convention used in other commands (which use 'trust-zone-id'). Consider aligning the flag name for consistency.

f.StringSliceVar(&opts.trustzoneIDs, "trustzone-id", []string{}, "Trust zone IDs to uninstall, or all if none is specified")

cmd/cofidectl/cmd/trustzone/trustzone_test.go

meyskens · 2025-05-21T11:17:42Z

Squased the pile of 43 commits into one, should be ready for review (sorry for the size, this had a huge domino effect)

markgoddard

It's looking better now I think. Haven't done a full review so there may be more things.

One thing to try is git grep -C3 nolint:staticcheck. This catches some cases where we're still using the name-based references (and a few other unrelated things)

cmd/cofidectl/cmd/apbinding/apbinding.go

pkg/plugin/local/local.go

This makes cofidctl use UUIDs everywhere instead of names. It also includes the needed API changes and CLI changes to make this happen Signed-off-by: Maartje Eyskens <maartje@eyskens.me>

meyskens force-pushed the me/reference-by-id branch 3 times, most recently from 81c7da1 to 156ee6e Compare April 25, 2025 13:58

meyskens marked this pull request as ready for review April 25, 2025 15:04

markgoddard self-requested a review May 9, 2025 15:57

markgoddard assigned meyskens and markgoddard May 9, 2025

markgoddard added this to the m21 milestone May 9, 2025

meyskens requested a review from Copilot May 14, 2025 14:23

Copilot AI reviewed May 14, 2025

View reviewed changes

cmd/cofidectl/cmd/workload/workload.go Outdated Show resolved Hide resolved

cmd/cofidectl/cmd/cluster/cluster.go Show resolved Hide resolved

meyskens force-pushed the me/reference-by-id branch 3 times, most recently from 85c8ce0 to b6cdd98 Compare May 15, 2025 12:43

meyskens requested a review from Copilot May 15, 2025 14:25

Copilot AI reviewed May 15, 2025

View reviewed changes

cmd/cofidectl/cmd/workload/workload.go Outdated Show resolved Hide resolved

cmd/cofidectl/cmd/federation/federation.go Outdated Show resolved Hide resolved

meyskens requested a review from Copilot May 15, 2025 14:32

Copilot AI reviewed May 15, 2025

View reviewed changes

cmd/cofidectl/cmd/trustzone/trustzone_test.go Outdated Show resolved Hide resolved

markgoddard mentioned this pull request May 20, 2025

fix: federation deletion with local data source #237

Merged

meyskens force-pushed the me/reference-by-id branch from 12d0b92 to 12ab009 Compare May 21, 2025 10:43

markgoddard reviewed May 22, 2025

View reviewed changes

cmd/cofidectl/cmd/apbinding/apbinding.go Show resolved Hide resolved

pkg/plugin/local/local.go Outdated Show resolved Hide resolved

meyskens force-pushed the me/reference-by-id branch 3 times, most recently from 5e31938 to 6bf9704 Compare May 26, 2025 10:06

Use ID fields for internal referencing in local source

8627d9d

This makes cofidctl use UUIDs everywhere instead of names. It also includes the needed API changes and CLI changes to make this happen Signed-off-by: Maartje Eyskens <maartje@eyskens.me>

meyskens force-pushed the me/reference-by-id branch from 6bf9704 to 8627d9d Compare May 26, 2025 10:14

github-actions bot added the chore label May 26, 2025

meyskens mentioned this pull request May 26, 2025

chore(deps): bump github.com/cofide/cofide-api-sdk from 0.19.0 to 0.20.0 #233

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Use ID fields for internal referencing in local source #211

Use ID fields for internal referencing in local source #211

Uh oh!

meyskens commented Apr 24, 2025 •

edited

Loading

Uh oh!

meyskens commented Apr 25, 2025

Uh oh!

markgoddard commented May 9, 2025

Uh oh!

meyskens commented May 14, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

meyskens commented May 21, 2025

Uh oh!

markgoddard left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Use ID fields for internal referencing in local source #211

Are you sure you want to change the base?

Use ID fields for internal referencing in local source #211

Uh oh!

Conversation

meyskens commented Apr 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

meyskens commented Apr 25, 2025

Uh oh!

markgoddard commented May 9, 2025

Uh oh!

meyskens commented May 14, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

meyskens commented May 21, 2025

Uh oh!

markgoddard left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

meyskens commented Apr 24, 2025 •

edited

Loading