✨ Must include Dockerfile in declarative plugin #2318
Conversation
k8s-ci-robot
added
cncf-cla: yes
justinsb
changed the title
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: justinsb The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
justinsb
force-pushed
the
dockerfile_must_copy_channels
branch
4 times, most recently
from
1609065 to
8054b0b
Compare
k8s-ci-robot
added
size/L
We need to copy the channels directory in the Dockerfile, particularly now that we're running with non-root permissions and it is no longer a one-liner.
justinsb
force-pushed
the
dockerfile_must_copy_channels
branch
from
8054b0b to
2108c36
Compare
|
APIDiff looks to be a false-failure. |
|
@camilamacedo86 should I try to work around the APIDIff incompatible changes on the Plugin? I don't think this API is user-facing. |
|
Hi @justinsb, Could you please rebase it with the master and run make generate for we are able to check? Also, feel free to ping me in the slack when it passes it the tests for we are able to move forward. |
There was a problem hiding this comment.
Hi @justinsb,
The contribution is great 🥇 Just a few nits.
See that, the declarative plugin is used to scaffold apis. See: https://book.kubebuilder.io/plugins/plugins.html#plugins
Example:
kubebuider create api [options] --plugins=go/v3,declarative/v1
So, if it requires changes into the Dockerfile we need to ensure that these changes will be performed on the create API subcommand as well.
Also, we need to add the scaffolds inside of the scaffold dir.
Could you please check it out?
| return nil | ||
| } | ||
|
|
||
| func (p *initSubcommand) Scaffold(fs machinery.Filesystem) error { |
There was a problem hiding this comment.
The scaffold would need to be the inside of the scaffold dir. (e.g: https://github.com/kubernetes-sigs/kubebuilder/blob/master/pkg/plugins/golang/v3/scaffolds/init.go)
| @@ -49,6 +50,9 @@ func (Plugin) Version() plugin.Version { return pluginVersion } | |||
| // SupportedProjectVersions returns an array with all project versions supported by the plugin | |||
| func (Plugin) SupportedProjectVersions() []config.Version { return supportedProjectVersions } | |||
|
|
|||
| // GetInitSubcommand will return the subcommand which is responsible for initializing and common scaffolding | |||
| func (p Plugin) GetInitSubcommand() plugin.InitSubcommand { return &p.initSubcommand } | |||
|
|
|||
| // GetCreateAPISubcommand will return the subcommand which is responsible for scaffolding apis | |||
| func (p Plugin) GetCreateAPISubcommand() plugin.CreateAPISubcommand { return &p.createAPISubcommand } | |||
There was a problem hiding this comment.
The declarative plugin can also be called via `create api --plugin' so its implementation shows not accurate.
Should we not be implementing it in the create API subcommand?
Seems that we would need to replace the dockerfile when we call create api --plugin as well.
camilamacedo86
added
the
help wanted
k8s-ci-robot
added
the
needs-rebase
|
@justinsb: PR needs rebase. 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. |
We need to copy the channels directory in the Dockerfile, particularly now that we're running with non-root permissions and it is no longer a one-liner.
Add Dockerfile generation.