Skip to content

fix: pass Ansible secret extra vars as JSON - #4250

Open
Iceman-ZP wants to merge 1 commit into
semaphoreui:developfrom
Iceman-ZP:fix/4249-ansible-secret-extra-vars
Open

Iceman-ZP wants to merge 1 commit into
semaphoreui:developfrom
Iceman-ZP:fix/4249-ansible-secret-extra-vars

Conversation

@Iceman-ZP

@Iceman-ZP Iceman-ZP commented Sep 18, 2026

Copy link
Copy Markdown

What does this PR do?

Fixes Ansible Variable Group secret variables being passed as separate --extra-vars name=value arguments.

Secret variables of type var are now merged into the existing JSON extra-vars payload before it is serialized and passed to Ansible.

This preserves values containing spaces and other complex content.

Environment secrets of type env are unchanged and continue to be passed as environment variables.

Why?

Regular extra variables are already passed to Ansible as JSON, but Variable Group secret variables were appended separately using:

--extra-vars name=value

This can cause Ansible to parse values containing spaces incorrectly.

Fixes #4249.

Behavior

The change preserves:

  • secret variable precedence over regular extra variables;
  • EnvironmentSecretEnv behavior;
  • a single JSON --extra-vars payload.

Tests

Added a regression test covering:

  • secret values containing spaces;
  • multiline secret values;
  • secret variable override precedence;
  • exclusion of EnvironmentSecretEnv values;
  • use of a single JSON extra-vars payload.

Tested with:

go test ./services/tasks -run TestGetPlaybookArgs_EnvironmentSecretVarsUseJSON -count=1

and:

go test ./services/tasks -count=1

Both pass on the current develop branch.

Summary by CodeRabbit

  • Bug Fixes
    • Improved handling of database secret variables during task execution.
    • Secret values, including those containing spaces or multiple lines, are now passed reliably as part of the consolidated variable payload.
    • Secret variables correctly take precedence over plain variables with the same name.
    • Secrets intended for environment-level configuration are excluded from task variables.

@coderabbitai

coderabbitai Bot commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 62bdf00c-7880-4b9f-ada5-cf549bfc08dd

📥 Commits

Reviewing files that changed from the base of the PR and between 83bddaf and 94a5556.

📒 Files selected for processing (2)
  • services/tasks/local_executor.go
  • services/tasks/local_executor_test.go

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

Environment variable secrets are merged into the JSON --extra-vars payload. Separate name=value arguments are removed. Tests verify complex values, secret precedence, and exclusion of environment-target secrets.

Changes

Environment secret transport

Layer / File(s) Summary
Consolidated JSON extra-vars flow
services/tasks/local_executor.go, services/tasks/local_executor_test.go
Variable-type environment secrets are added to the JSON extra-vars map. Separate secret arguments are removed. Tests verify spaces, multiline values, precedence, and env-target exclusion.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: fiftin

Merge Risk: ⚪ Minimal · up to 94a55

Secret variable values are now transported through the consolidated JSON payload while environment-only secrets remain excluded. No actionable merge risk remains.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: passing Ansible secret extra variables through the JSON payload.
Linked Issues check ✅ Passed Issue #4249 requires EnvironmentSecretVar values to use the JSON --extra-vars payload, retain secret precedence, keep EnvironmentSecretEnv values out of that payload, and remove separate `name=v…
Out of Scope Changes check ✅ Passed The changes are limited to Ansible extra-variable construction in services/tasks/local_executor.go and its regression test in services/tasks/local_executor_test.go. The implementation and test dir…
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

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

@fiftin

fiftin commented Sep 20, 2026

Copy link
Copy Markdown
Collaborator

@Iceman-ZP as I remember the env is stored in a database in the task table.

Secrets shouldn't be stored in the database.

@Iceman-ZP

Iceman-ZP commented Sep 21, 2026 via email

Copy link
Copy Markdown
Author

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.

Problem: Ansible secret extra-vars are parsed incorrectly when values contain spaces

2 participants