Skip to content

fix: apply invocation config to workflow Data after callback#606

Open
danskmt wants to merge 1 commit into
mainfrom
chore/CLI-1449-ufm-dataimpl-config
Open

fix: apply invocation config to workflow Data after callback#606
danskmt wants to merge 1 commit into
mainfrom
chore/CLI-1449-ufm-dataimpl-config

Conversation

@danskmt
Copy link
Copy Markdown
Contributor

@danskmt danskmt commented May 11, 2026

Description

Workflows can return Data created with workflow.NewData without WithConfiguration, so IN_MEMORY_THRESHOLD_BYTES and TEMP_DIR_PATH were not applied to those payloads.

After each workflow callback returns, EngineImpl.Invoke calls applyConfiguration on each *DataImpl in the output slice so in-memory payloads can spill to the configured temp directory when above threshold. applyConfiguration is package-private on DataImpl (same package as the engine) so it is not exported from the workflow package.

Adds tests in dataimpl_test.go for spill and no-op cases, and in engine_test.go for behavior after Invoke.

Checklist

  • Tests added and all succeed (make test)
  • Regenerated mocks, etc. (make generate)
  • Linted (make lint)
  • Test your changes work for the CLI
    1. Clone / pull the latest CLI main.
    2. Run go get github.com/snyk/go-application-framework@YOUR_LATEST_GAF_COMMIT in the cliv2 directory.
      • Tip: for local testing, you can uncomment the line near the bottom of the CLI's go.mod to point to your local GAF code.
    3. Run go mod tidy in the cliv2 directory.
    4. Run the CLI tests and do any required manual testing.
    5. Open a PR in the CLI repo now with the go.mod and go.sum changes.
    • Once this PR is merged, repeat these steps, but pointing to the latest GAF commit on main and update your CLI PR.

PR in CLI snyk/cli#6792

Where reviewers should start

  • pkg/workflow/dataimpl.goapplyConfiguration
  • pkg/workflow/engineimpl.go — post-process loop after callback

Risk assessment

Low. Default production behavior is unchanged unless spill-related settings are enabled; when they are, returned Data placement aligns with engine configuration instead of leaving eligible payloads in memory.

@snyk-io
Copy link
Copy Markdown

snyk-io Bot commented May 11, 2026

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues
Code Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@snyk-io
Copy link
Copy Markdown

snyk-io Bot commented May 11, 2026

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues
Code Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@danskmt danskmt marked this pull request as ready for review May 11, 2026 13:04
@danskmt danskmt requested review from a team as code owners May 11, 2026 13:04
@snyk-pr-review-bot

This comment has been minimized.

Comment thread pkg/workflow/dataimpl.go Outdated
@danskmt danskmt force-pushed the chore/CLI-1449-ufm-dataimpl-config branch from b804c2c to 2bb8c90 Compare May 12, 2026 13:05
@snyk-pr-review-bot

This comment has been minimized.

@danskmt danskmt force-pushed the chore/CLI-1449-ufm-dataimpl-config branch from 2bb8c90 to 89017c8 Compare May 12, 2026 13:27
@snyk-pr-review-bot

This comment has been minimized.

@danskmt danskmt force-pushed the chore/CLI-1449-ufm-dataimpl-config branch from 89017c8 to 86de328 Compare May 12, 2026 13:49
@snyk-pr-review-bot
Copy link
Copy Markdown

PR Reviewer Guide 🔍

🧪 PR contains tests
🔒 No security concerns identified
⚡ No major issues detected
📚 Repository Context Analyzed

This review considered 12 relevant code sections from 8 files (average relevance: 0.95)

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.

2 participants