Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🏃 Homogenize commands through Scaffolder interface #1330

Merged

Conversation

Adirio
Copy link
Contributor

@Adirio Adirio commented Jan 23, 2020

Description

This PR defines a Scaffolder interface with a single method:

type Scaffolder interface {
	Scaffold()
}

This interface is used as the API between the cmd and the pkg/scaffold packages. This allows to provide a clearer limit between the two packages, as previously the limit was not clear and logic that should be placed in pkg/scaffold could be found in cmd (e.g., webhook scaffolding was being done directly in cmd).

Additionally the option structs for each command has also been homogenized. An unexported interface has been used for this purpose:

type commandOptions interface {
	// bindFlags binds the command flags to the fields in the options struct
	bindFlags(command *cobra.Command)

	// The following steps define a generic logic to follow when developing new commands. Some steps may be no-ops.
	// - Step 1: load the config failing if expected but not found or if not expected but found
	loadConfig() (*config.Config, error)
	// - Step 2: verify that the command can be run (e.g., go version, project version, arguments, ...)
	validate(*config.Config) error
	// - Step 3: create the Scaffolder instance
	scaffolder(*config.Config) (scaffold.Scaffolder, error)
	// - Step 4: call the Scaffold method of the Scaffolder instance
	// Doesn't need any method
	// - Step 5: finish the command execution
	postScaffold(*config.Config) error
}
}

The logic flow described for the run method has been used for every command. In some cases, some steps are simply no-ops, like the 5th step for the edit command.

Motivation

This PR is one of the main contributors to #1218.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 23, 2020
@k8s-ci-robot
Copy link
Contributor

Hi @Adirio. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 23, 2020
@Adirio Adirio force-pushed the scaffold-enhancement/scaffolder branch from 27f745e to 5e1cf20 Compare January 23, 2020 11:58
@Adirio Adirio force-pushed the scaffold-enhancement/scaffolder branch 6 times, most recently from b61fb5e to df222db Compare January 23, 2020 14:45
@Adirio Adirio force-pushed the scaffold-enhancement/scaffolder branch from df222db to e07ad64 Compare January 24, 2020 14:14
@DirectXMan12
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 27, 2020
Copy link
Contributor

@DirectXMan12 DirectXMan12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good. Is there any way that we can put the steps into an interface / helper so we don't have to repeat them in every run method?

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 27, 2020
@Adirio
Copy link
Contributor Author

Adirio commented Jan 27, 2020

Is there any way that we can put the steps into an interface / helper so we don't have to repeat them in every run method?

@DirectXMan12 Something like the following?

type commandOptions interface {
	bindFlags(cmd *cobra.Command)
	loadConfig() (*Config, error)
	validate(*config.Config) error
	scaffolder(*config.Config) (Scaffolder, error)
	postScaffold(*config.Config) error
}

func run(options commandOptions) error {
	// Step 1: load config
	config, err := options.loadConfig()
	if err != nil {
		return err
	}

	// Step 2: validate
	if err := options.validate(config); err != nil {
		return err
	}

	// Step 3: create scaffolder
	scaffolder, err := options.scaffolder(config)
	if err != nil {
		return err
	}

	// Step 4: scaffold
	if err := scaffolder.Scaffold(); err != nil {
		return err
	}

	// Step 5: finish
	if err := options.postScaffold(); err != nil {
		return err
	}
}

@Adirio Adirio force-pushed the scaffold-enhancement/scaffolder branch from e07ad64 to a373c63 Compare January 28, 2020 14:43
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 28, 2020
@Adirio Adirio force-pushed the scaffold-enhancement/scaffolder branch from a373c63 to abd4bda Compare January 28, 2020 15:02
@Adirio
Copy link
Contributor Author

Adirio commented Jan 28, 2020

Didn't expect the golden test to check file permission, but it does.

@Adirio
Copy link
Contributor Author

Adirio commented Jan 28, 2020

@DirectXMan12 I made a separate commit with the helper as I described here.

@Adirio Adirio force-pushed the scaffold-enhancement/scaffolder branch from 22269f0 to 5ecdb8b Compare January 28, 2020 15:36
@DirectXMan12
Copy link
Contributor

looks good once it's rebased

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 28, 2020
@Adirio Adirio force-pushed the scaffold-enhancement/scaffolder branch from 5ecdb8b to 4ddaf11 Compare January 28, 2020 19:33
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 28, 2020
@Adirio Adirio force-pushed the scaffold-enhancement/scaffolder branch from 4ddaf11 to 5627ff6 Compare January 29, 2020 08:26
Signed-off-by: Adrian Orive <adrian.orive.oneca@gmail.com>
Copy link
Member

@mengqiy mengqiy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one nit.
Other bits LGTM.

cmd/api.go Show resolved Hide resolved
@mengqiy
Copy link
Member

mengqiy commented Jan 29, 2020

This is a great refactor! Thanks for working on this.

@mengqiy mengqiy changed the title Homogenize commands through Scaffolder interface 🏃 Homogenize commands through Scaffolder interface Jan 29, 2020
@mengqiy
Copy link
Member

mengqiy commented Jan 29, 2020

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 29, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Adirio, mengqiy

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 29, 2020
@k8s-ci-robot k8s-ci-robot merged commit bb5830b into kubernetes-sigs:master Jan 29, 2020
@Adirio Adirio deleted the scaffold-enhancement/scaffolder branch January 29, 2020 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants