Skip to content

Conversation

@javirln
Copy link
Member

@javirln javirln commented Dec 12, 2025

Implemented automatic detection and collection of Pull Request (GitHub) and Merge Request (GitLab) metadata during attestation initialization when running in CI/CD PR/MR contexts.

Key Features

Automatic PR/MR Detection

  • Detects GitHub PR context from GITHUB_EVENT_NAME and GITHUB_EVENT_PATH
  • Detects GitLab MR context from CI_PIPELINE_SOURCE and merge request environment variables
  • Extracts metadata: platform, type, number, title, description, branches, URL, author

New Material Type: CHAINLOOP_PR_INFO

  • Added CHAINLOOP_PR_INFO material type to workflow contracts
  • Automatically collected during attestation init in PR/MR contexts
  • Stored as JSON evidence with schema validation

Schema Generation & Validation

  • Created internal/prinfo package with typed Go structs
  • Programmatic JSON schema generation using jsonschema annotations
  • Schema validation enforces required fields and enum constraints
  • Schema: https://schemas.chainloop.dev/prinfo/1.0/pr-info.schema.json

Implementation Details

  • pkg/attestation/crafter/prmetadata.go: Detection logic for GitHub/GitLab
  • pkg/attestation/crafter/materials/chainloop_pr_info.go: Material crafter with validation
  • app/cli/pkg/action/attestation_init.go: Auto-collection during init
  • Best-effort collection - failures don't block attestation

@javirln javirln self-assigned this Dec 12, 2025
@javirln javirln requested review from Piskoo and migmartri and removed request for Piskoo December 12, 2025 10:25
Signed-off-by: Javier Rodriguez <[email protected]>
Signed-off-by: Javier Rodriguez <[email protected]>
Signed-off-by: Javier Rodriguez <[email protected]>
@javirln javirln requested a review from jiparis December 15, 2025 09:15
@jiparis
Copy link
Member

jiparis commented Dec 15, 2025

Probably it's been already discussed:

  • why a new material? We have attestation metadata for other related properties in the crafting state (attestation)
  • why in attestation init and not in push?
  • also, should it be optional?

// Log warning but don't fail - will fall back to inline storage
action.Logger.Warn().Err(err).Msg("failed to get CAS credentials for PR metadata, will store inline")
} else {
b := creds.GetResult().GetBackend()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't we have this CAS logic already elsewhere?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have this logic on attestation push

Copy link
Member

@jiparis jiparis Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see it's also in attestation add, so maybe it can we moved to a common function and reuse it. Something like:

var casBackend = &casclient.CASBackend{Name: "not-set"}
if !action.dryRun && attestationID != "" {
    casBackend, err = getCASBackend(ctx, attestationID) 
    ...
}

Signed-off-by: Javier Rodriguez <[email protected]>
Signed-off-by: Javier Rodriguez <[email protected]>
Signed-off-by: Javier Rodriguez <[email protected]>
Signed-off-by: Javier Rodriguez <[email protected]>
Signed-off-by: Javier Rodriguez <[email protected]>

// getCASBackend tries to get CAS upload credentials and set up a CAS client
// If strict is true, errors are returned; if false, warnings are logged and nil error is returned
func getCASBackend(ctx context.Context, client pb.AttestationServiceClient, workflowRunID, casCAPath, casURI string, casConnectionInsecure bool, logger zerolog.Logger, casBackend *casclient.CASBackend) (func() error, error) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jiparis the refactor

Signed-off-by: Javier Rodriguez <[email protected]>
Signed-off-by: Javier Rodriguez <[email protected]>
Signed-off-by: Javier Rodriguez <[email protected]>
@javirln javirln merged commit 4d6157b into chainloop-dev:main Dec 15, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants