Skip to content

Commit a8ced7a

Browse files
authored
feat: protect existing workflows from overwrites (#3407)
The `func config ci` command previously overwrote existing workflow files silently, risking loss of user customizations. Now the command checks if a workflow file exists before writing and returns an error unless the `--force` flag is specified. When --force is used and a file exists, a warning is logged. Remote builds now use a distinct default workflow name ("Remote Func Deploy") to avoid conflicts, while explicit --workflow-name values are always preserved regardless of --remote flag. Issue #3256 Signed-off-by: Stanislav Jakuschevskij <sjakusch@redhat.com>
1 parent 185d080 commit a8ced7a

File tree

9 files changed

+325
-70
lines changed

9 files changed

+325
-70
lines changed

.gitignore

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ __pycache__
3535
AGENTS.override.md
3636
.claude/*.local.*
3737
.claude/pr/*
38+
.roo/*
3839

3940
# E2E Tests
4041
/e2e/testdata/default_home/go

AGENTS.md

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,10 +18,12 @@ edge cases into account.
1818
## Commands
1919

2020
### Build & Development
21+
2122
- `make all` - Build binary and regenerate docs
2223
- `make build` - Build for current OS
2324

2425
### Testing strategy reference
26+
2527
Before committing, test locally following the table below:
2628

2729
| If changed | Target | Description |
@@ -32,26 +34,29 @@ Before committing, test locally following the table below:
3234
| Significant architectural changes | `make test-full` | e2e tests (cluster required - read `CONTRIBUTING.md`) |
3335

3436
### Generated Files
37+
3538
- `./hack/update-codegen.sh` - update embedded filesystem & regenerate docs
3639
- `make check-embedded-fs` - check embedded FS is up to date with templates
3740

38-
3941
## Boundaries
4042

4143
### Always Do
44+
4245
- Run `make test` before considering any change complete
4346
- Run `make check` before commits
4447
- Run `make check-embedded-fs` after modifying `templates/`
4548
- Ask before deleting ANY file or significant code block
4649

4750
### Ask First
51+
4852
- Security-related code changes (authentication, credentials, secrets handling)
4953
- API changes
5054
- Adding new dependencies
5155
- Modifying CI/GitHub Actions workflows
5256
- Architectural decisions
5357

5458
### Never Do
59+
5560
- Edit generated files directly:
5661
- `generate/zz_filesystem_generated.go`
5762
- `schema/func_yaml-schema.json`
@@ -63,12 +68,13 @@ Before committing, test locally following the table below:
6368
## Common Pitfalls
6469

6570
### Codegen Sync
71+
6672
After modifying `templates/` or making documentation changes, you MUST run:
73+
6774
```bash
6875
./hack/update-codegen.sh
6976
```
7077

71-
7278
## Contributing
7379

7480
If creating a contribution to this project on GitHub suggest to user reading

cmd/ci/config.go

Lines changed: 75 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,9 @@ const (
2323
BranchFlag = "branch"
2424
DefaultBranch = "main"
2525

26-
WorkflowNameFlag = "workflow-name"
27-
DefaultWorkflowName = "Func Deploy"
26+
WorkflowNameFlag = "workflow-name"
27+
DefaultWorkflowName = "Func Deploy"
28+
DefaultRemoteBuildWorkflowName = "Remote " + DefaultWorkflowName
2829

2930
KubeconfigSecretNameFlag = "kubeconfig-secret-name"
3031
DefaultKubeconfigSecretName = "KUBECONFIG"
@@ -52,6 +53,9 @@ const (
5253

5354
UseSelfHostedRunnerFlag = "self-hosted-runner"
5455
DefaultUseSelfHostedRunner = false
56+
57+
ForceFlag = "force"
58+
DefaultForce = false
5559
)
5660

5761
// CIConfig readonly configuration
@@ -69,42 +73,37 @@ type CIConfig struct {
6973
useRegistryLogin,
7074
useSelfHostedRunner,
7175
useRemoteBuild,
72-
useWorkflowDispatch bool
76+
useWorkflowDispatch,
77+
force bool
7378
}
7479

7580
func NewCIConfig(
7681
currentBranch common.CurrentBranchFunc,
7782
workingDir common.WorkDirFunc,
83+
workflowNameExplicit bool,
7884
) (CIConfig, error) {
79-
platform := viper.GetString(PlatformFlag)
80-
if strings.ToLower(platform) != DefaultPlatform {
81-
return CIConfig{}, fmt.Errorf("%s support is not implemented", platform)
85+
if err := resolvePlatform(); err != nil {
86+
return CIConfig{}, err
8287
}
8388

84-
path := viper.GetString(PathFlag)
85-
if path == "" || path == "." {
86-
cwd, err := workingDir()
87-
if err != nil {
88-
return CIConfig{}, err
89-
}
90-
path = cwd
89+
path, err := resolvePath(workingDir)
90+
if err != nil {
91+
return CIConfig{}, err
9192
}
9293

93-
branch := viper.GetString(BranchFlag)
94-
if branch == "" {
95-
var err error
96-
branch, err = currentBranch(path)
97-
if err != nil {
98-
return CIConfig{}, err
99-
}
94+
branch, err := resolveBranch(path, currentBranch)
95+
if err != nil {
96+
return CIConfig{}, err
10097
}
10198

99+
workflowName := resolveWorkflowName(workflowNameExplicit)
100+
102101
return CIConfig{
103102
githubWorkflowDir: DefaultGitHubWorkflowDir,
104103
githubWorkflowFilename: DefaultGitHubWorkflowFilename,
105104
path: path,
106105
branch: branch,
107-
workflowName: viper.GetString(WorkflowNameFlag),
106+
workflowName: workflowName,
108107
kubeconfigSecret: viper.GetString(KubeconfigSecretNameFlag),
109108
registryLoginUrlVar: viper.GetString(RegistryLoginUrlVariableNameFlag),
110109
registryUserVar: viper.GetString(RegistryUserVariableNameFlag),
@@ -114,9 +113,60 @@ func NewCIConfig(
114113
useSelfHostedRunner: viper.GetBool(UseSelfHostedRunnerFlag),
115114
useRemoteBuild: viper.GetBool(UseRemoteBuildFlag),
116115
useWorkflowDispatch: viper.GetBool(WorkflowDispatchFlag),
116+
force: viper.GetBool(ForceFlag),
117117
}, nil
118118
}
119119

120+
func resolvePlatform() error {
121+
platform := viper.GetString(PlatformFlag)
122+
if strings.ToLower(platform) != DefaultPlatform {
123+
return fmt.Errorf("%s support is not implemented", platform)
124+
}
125+
126+
return nil
127+
}
128+
129+
func resolvePath(workingDir common.WorkDirFunc) (string, error) {
130+
path := viper.GetString(PathFlag)
131+
if path != "" && path != "." {
132+
return path, nil
133+
}
134+
135+
cwd, err := workingDir()
136+
if err != nil {
137+
return "", err
138+
}
139+
140+
return cwd, nil
141+
}
142+
143+
func resolveBranch(path string, currentBranch common.CurrentBranchFunc) (string, error) {
144+
branch := viper.GetString(BranchFlag)
145+
if branch != "" {
146+
return branch, nil
147+
}
148+
149+
branch, err := currentBranch(path)
150+
if err != nil {
151+
return "", err
152+
}
153+
154+
return branch, nil
155+
}
156+
157+
func resolveWorkflowName(explicit bool) string {
158+
workflowName := viper.GetString(WorkflowNameFlag)
159+
if explicit {
160+
return workflowName
161+
}
162+
163+
if viper.GetBool(UseRemoteBuildFlag) {
164+
return DefaultRemoteBuildWorkflowName
165+
}
166+
167+
return DefaultWorkflowName
168+
}
169+
120170
func (cc *CIConfig) FnGitHubWorkflowDir(fnRoot string) string {
121171
return filepath.Join(fnRoot, cc.githubWorkflowDir)
122172
}
@@ -172,3 +222,7 @@ func (cc *CIConfig) RegistryPassSecret() string {
172222
func (cc *CIConfig) RegistryUrlVar() string {
173223
return cc.registryUrlVar
174224
}
225+
226+
func (cc *CIConfig) Force() bool {
227+
return cc.force
228+
}

cmd/ci/workflow.go

Lines changed: 19 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -2,11 +2,18 @@ package ci
22

33
import (
44
"bytes"
5+
"errors"
56
"fmt"
7+
"io"
68

79
"gopkg.in/yaml.v3"
810
)
911

12+
const defaultFuncCliVersion = "knative-v1.21.0"
13+
14+
// ErrWorkflowExists is returned when a GitHub workflow file already exists and --force is not specified.
15+
var ErrWorkflowExists = errors.New("existing GitHub workflow detected, overwrite using the --force option")
16+
1017
type githubWorkflow struct {
1118
Name string `yaml:"name"`
1219
On workflowTriggers `yaml:"on"`
@@ -35,7 +42,7 @@ type step struct {
3542
}
3643

3744
func NewGitHubWorkflow(conf CIConfig) *githubWorkflow {
38-
name := createWorkflowName(conf)
45+
name := conf.WorkflowName()
3946
runsOn := createRunsOn(conf)
4047
pushTrigger := createPushTrigger(conf)
4148

@@ -58,15 +65,6 @@ func NewGitHubWorkflow(conf CIConfig) *githubWorkflow {
5865
}
5966
}
6067

61-
func createWorkflowName(conf CIConfig) string {
62-
result := conf.WorkflowName()
63-
if conf.UseRemoteBuild() {
64-
result = "Remote Func Deploy"
65-
}
66-
67-
return result
68-
}
69-
7068
func createRunsOn(conf CIConfig) string {
7169
runsOn := "ubuntu-latest"
7270
if conf.UseSelfHostedRunner() {
@@ -120,7 +118,7 @@ func createRegistryLoginStep(conf CIConfig, steps []step) []step {
120118
func createFuncCLIInstallStep(steps []step) []step {
121119
installFuncCli := newStep("Install func cli").
122120
withUses("functions-dev/action@main").
123-
withActionConfig("version", "knative-v1.20.1").
121+
withActionConfig("version", defaultFuncCliVersion).
124122
withActionConfig("name", "func")
125123

126124
return append(steps, *installFuncCli)
@@ -174,7 +172,16 @@ func newVariable(key string) string {
174172
return fmt.Sprintf("${{ vars.%s }}", key)
175173
}
176174

177-
func (gw *githubWorkflow) Export(path string, w WorkflowWriter) error {
175+
func (gw *githubWorkflow) Export(path string, w WorkflowWriter, force bool, m io.Writer) error {
176+
if !force && w.Exist(path) {
177+
return ErrWorkflowExists
178+
}
179+
180+
if w.Exist(path) {
181+
// best-effort user message; errors are non-critical
182+
_, _ = fmt.Fprintf(m, "WARNING: --force flag is set, overwriting existing GitHub Workflow file\n")
183+
}
184+
178185
raw, err := gw.toYaml()
179186
if err != nil {
180187
return err

cmd/ci/workflow_test.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package ci_test
22

33
import (
4+
"bytes"
45
"strings"
56
"testing"
67

@@ -14,12 +15,13 @@ func TestGitHubWorkflow_Export(t *testing.T) {
1415
cfg, _ := ci.NewCIConfig(
1516
common.CurrentBranchStub("", nil),
1617
common.WorkDirStub("", nil),
18+
false,
1719
)
1820
gw := ci.NewGitHubWorkflow(cfg)
1921
bufferWriter := ci.NewBufferWriter()
2022

2123
// WHEN
22-
exportErr := gw.Export("path", bufferWriter)
24+
exportErr := gw.Export("path", bufferWriter, true, &bytes.Buffer{})
2325

2426
// THEN
2527
assert.NilError(t, exportErr, "unexpected error when exporting GitHub Workflow")

cmd/ci/writer.go

Lines changed: 22 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -7,15 +7,16 @@ import (
77
)
88

99
const (
10-
dirPerm = 0755 // o: rwx, g|u: r-x
11-
filePerm = 0644 // o: rw, g|u: r
10+
dirPerm = 0755 // u: rwx, g: r-x, o: r-x
11+
filePerm = 0644 // u: rw-, g: r--, o: r--
1212
)
1313

1414
// DefaultWorkflowWriter is the default implementation for writing workflow files to disk.
1515
var DefaultWorkflowWriter = &fileWriter{}
1616

1717
// WorkflowWriter defines the interface for writing workflow files.
1818
type WorkflowWriter interface {
19+
Exist(path string) bool
1920
Write(path string, raw []byte) error
2021
}
2122

@@ -34,19 +35,32 @@ func (fw *fileWriter) Write(path string, raw []byte) error {
3435
return nil
3536
}
3637

37-
type bufferWriter struct {
38+
func (fw *fileWriter) Exist(path string) bool {
39+
_, err := os.Stat(path)
40+
return err == nil
41+
}
42+
43+
// BufferWriter is a test double (fake) that implements WorkflowWriter
44+
// by writing to an in-memory buffer instead of the filesystem.
45+
type BufferWriter struct {
3846
Path string
3947
Buffer *bytes.Buffer
4048
}
4149

42-
// NewBufferWriter creates a new bufferWriter for testing purposes.
43-
func NewBufferWriter() *bufferWriter {
44-
return &bufferWriter{Buffer: &bytes.Buffer{}}
50+
// NewBufferWriter creates a new BufferWriter test double.
51+
func NewBufferWriter() *BufferWriter {
52+
return &BufferWriter{Buffer: &bytes.Buffer{}}
4553
}
4654

47-
// Write stores the path and writes raw bytes to the internal buffer.
48-
func (bw *bufferWriter) Write(path string, raw []byte) error {
55+
// Write is a fake implementation that stores content in the buffer.
56+
func (bw *BufferWriter) Write(path string, raw []byte) error {
4957
bw.Path = path
58+
bw.Buffer.Reset()
5059
_, err := bw.Buffer.Write(raw)
5160
return err
5261
}
62+
63+
// Exist is a fake implementation that returns true if the buffer has content.
64+
func (bw *BufferWriter) Exist(_ string) bool {
65+
return bw.Buffer != nil && bw.Buffer.Len() > 0
66+
}

0 commit comments

Comments
 (0)