Refactor: Move CI workflow generation from cmd/ci to pkg/ci/github.#3572
Refactor: Move CI workflow generation from cmd/ci to pkg/ci/github.#3572twoGiants wants to merge 1 commit intoknative:mainfrom
cmd/ci to pkg/ci/github.#3572Conversation
|
Skipping CI for Draft Pull Request. |
knative-prow
bot
added
do-not-merge/work-in-progress
|
[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
size/XXL
knative-prow-robot
added
needs-rebase
twoGiants
force-pushed
the
issue-784-migration-to-pkg
branch
from
293736b to
9ea2b6e
Compare
knative-prow-robot
removed
the
needs-rebase
The CI workflow generation lived in cmd/ci/, tightly coupled to the command layer via viper flag reads and direct file I/O. This made it impossible to reuse or test the generator independently. Move workflow generation, printing, and writing into pkg/ci/github/ as a self-contained package. Introduce CI and PathWriter interfaces in pkg/functions/client.go so the client can orchestrate workflow generation without depending on the cmd layer. The cmd/config_ci.go command now resolves flags and delegates to client.GenerateCIWorkflow(). Also fix typos: --patform → --platform, functionl → function, proivdes → provides, precldes → precedes, sevices → services, suppot → support, Desribe → Describe, pipilines → pipelines, intance → instance. Issue knative#3256 Signed-off-by: Stanislav Jakuschevskij <sjakusch@redhat.com>
twoGiants
force-pushed
the
issue-784-migration-to-pkg
branch
from
9ea2b6e to
2a954f6
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3572 +/- ##
==========================================
+ Coverage 56.13% 56.37% +0.24%
==========================================
Files 180 181 +1
Lines 20465 20486 +21
==========================================
+ Hits 11489 11550 +61
+ Misses 7781 7744 -37
+ Partials 1195 1192 -3
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:
|
| fn.WithPathWriter(pathWriter), | ||
| fn.WithStdout(cmd.OutOrStdout()), |
There was a problem hiding this comment.
These could be constructor arguments on the CI generator implementation.
| DefaultForce = false | ||
| ) | ||
|
|
||
| type Config struct { |
There was a problem hiding this comment.
Perhaps turn these all (except FnRuntime and FnRoot) into members of the Generator, set as part of construction, and add verbose. Then, for example, if this struct is renamed WorkflowGeneratorOptions, then the constructor becomes simply: NewWorkflowGenerator(opts WorkflowGeneratorOptions) *WorkflowGenerator and the CI interface in client becomes:
Generate(ctx context.Context, f Function)
The implementation then pulls the FnRuntime and FnRoot from f, and everything else from it's WorkflowGeneratorOptions struct.
| } | ||
|
|
||
| type CI interface { | ||
| Generate(context.Context, any, PathWriter, io.Writer) error |
There was a problem hiding this comment.
This is definitely the right pattern; keeping everything neatly gated behind an interface, and dependencies held at bay.
Simply moving most of the config to the concrete implementation's constructor (as well as the writers), and we can keep only the universal settings here. That could yield an actualy gramatically correct interface:
"Generate a CI Workflow for function X" == Generate(context.Context, Function) error
| Start(context.Context, bool) error | ||
| } | ||
|
|
||
| type CI interface { |
There was a problem hiding this comment.
While I know it's not nearly as pretty, the Go idiom is to use something like "CIGenerator" here. That way it flows rather nicely:
generator := github.CIGenerator(...)
generator.Generate(ctx, f)
| assert.NilError(t, result.executeErr) | ||
| } | ||
|
|
||
| func TestNewConfigCICmd_WritesWorkflowFile(t *testing.T) { |
There was a problem hiding this comment.
The CLI's job is to translate a request into an API call, so if you use a mock CIGenerator here to ensure flags show up in that struct's constructor as-expected, and that it's Generate method is invoked when expected, then the testing here in the CLI is complete!
Then, you can rely on the tests in the github package to ensure that the implementation is correct (writes a workflow file, and writes it with the correct structure). That might simplify things here.
| @@ -362,7 +362,7 @@ | |||
| func TestNewConfigCICmd_ForceFlagOverwritesExistingWorkflow(t *testing.T) { | |||
There was a problem hiding this comment.
Here's a concrete example! Notice how the behavior is : the GitHub CI Generator overwrites an existing workflow file when force is enabled.
That's a test for the implementation itself. The CLI needs to make sure that that force is set to true if --force is provided, and leave the rest up to the implementation.
This tests actually looks a lot like an E2E. Where you'd actually call the cli command twice and confirm it overwrote interstital changes.
| Branch: %s | ||
| Builder: %s | ||
| Remote build: %s | ||
| Remote build %s |
There was a problem hiding this comment.
This looks like an inadvertent change
3 participants
Changes
cmd/ci/topkg/ci/github/as a self-contained, reusable packageCIandPathWriterinterfaces inpkg/functions/client.goso the client can orchestrate workflow generation without depending on the command layerGenerateCIWorkflowmethod to the client, routingfunc config cithroughclient.GenerateCIWorkflow()instead of calling the generator directly from the command--patform->--platform,functionl->function,proivdes->provides,precldes->precedes,sevices->services,suppot->support,Desribe->Describe,pipilines->pipelines,intance->instanceCIConfignow uses exported struct fields instead of getter methods, loaded upfront with runtime and root path encapsulated/kind enhancement
Relates to #3256
Release Note
Docs