Feature: Protect existing workflow files from accidental overwrites. #3407
Conversation
knative-prow-robot
added
the
needs-rebase
knative-prow
bot
added
the
size/L
twoGiants
force-pushed
the
issue-782-defensive-workflow-file-generation
branch
4 times, most recently
from
c7ebe59 to
ab5ed8f
Compare
knative-prow-robot
removed
the
needs-rebase
twoGiants
force-pushed
the
issue-782-defensive-workflow-file-generation
branch
from
ab5ed8f to
8162643
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3407 +/- ##
==========================================
- Coverage 54.60% 54.49% -0.11%
==========================================
Files 179 179
Lines 20228 20295 +67
==========================================
+ Hits 11045 11060 +15
- Misses 8011 8061 +50
- Partials 1172 1174 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
twoGiants
force-pushed
the
issue-782-defensive-workflow-file-generation
branch
from
8162643 to
8109c29
Compare
|
/cc @gauron99 @matejvasek @lkingland The PR is ready for review. Please read the description about the logging. |
|
/lgtm |
knative-prow
bot
added
the
do-not-merge/hold
The codecov tool is poorly configured or am I missing something? It's flagging the |
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. This gives
users explicit control over when to replace their existing workflows.
When --force is used and a workflow file exists, a warning
message is logged to inform the user that the existing file is being
overwritten.
The implementation introduces a Logger interface to cmd/common
with slog-based production implementation and a buffer-based test
double for verification.
Additionally, remote builds now use a distinct default workflow name
("Remote Func Deploy") to avoid conflicts.
Issue knative#3256
Signed-off-by: Stanislav Jakuschevskij <sjakusch@redhat.com>
twoGiants
force-pushed
the
issue-782-defensive-workflow-file-generation
branch
from
8109c29 to
6fb5f5e
Compare
|
New changes are detected. LGTM label has been removed. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: twoGiants The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
knative-prow
bot
added
the
do-not-merge/work-in-progress
| func resolveWorkflowName() string { | ||
| workflowName := viper.GetString(WorkflowNameFlag) | ||
| if workflowName != DefaultWorkflowName && workflowName != "" { | ||
| return workflowName | ||
| } | ||
|
|
||
| useRemoteBuild := viper.GetBool(UseRemoteBuildFlag) | ||
| if useRemoteBuild { | ||
| return DefaultRemoteBuildWorkflowName | ||
| } | ||
|
|
||
| return DefaultWorkflowName | ||
| } |
There was a problem hiding this comment.
So if I understand this right, if a user explicitly sets workflow-name="Func Deploy" (the default value), the function treats it as if no custom name was provided because of the workflowName != DefaultWorkflowName check. This means with --remote --workflow-name="Func Deploy", we'd get "Remote Func Deploy" instead of their explicitly requested "Func Deploy"?
Something like this might help:
workflowNameChanged := cmd.Flags().Changed(WorkflowNameFlag)
4 participants
Changes
--forceflag to explicitly allow overwriting existing workflow files--forceis used to overwrite existing workflowsLogger Clarifications
Here is some ground work for the upcoming
--verboseflag implementation. I introduce logging using an interface and maybe we can slowly start switching to such an approach.For now the interface has only one method but more are to follow. I am keeping it simple on purpose. The logger is also testable with a spy test double.
When implementing the
--verboselogging I will introduceVerboseInfo(...)andSetLevel()methods and a newVerboseInfoLevellog level for verbose logging and set it when the--verboseflag is used.It's a first draft/proposal for the introduction of logging to our project. I chose this path because we will be able to replace the logging implementation easily and also allow the user to provide his own logger if he uses
funcas library.[UPDATED]
This is how the logging is currently configured to look:
Now I am a bit unsure about it. It seems that what I am looking for here is not really logging in the traditional sense but information shown to the user. In a GUI that would be some notification message or an info text and not actual logs.
/kind enhancement
Relates to #3256
Release Note
Docs