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

Make files in `spec.platforms[]` optional #261

Merged

Conversation

@corneliusweig
Copy link
Contributor

commented Jul 16, 2019

Many plugins use the pattern to copy all files from the archive into the installation directory. Those plugins always have to specify a file copy operation such as

files:
- from: "*"
  to: "."

This PR makes the files field completely optional and defaults to the above copy specifier. As discussed in #135, the default copy spec may install too many files. The plugin developer docs therefore explicitly show such an antipattern example.

Close #135

@codecov-io

This comment has been minimized.

Copy link

commented Jul 16, 2019

Codecov Report

Merging #261 into master will increase coverage by 0.67%.
The diff coverage is 87.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #261      +/-   ##
==========================================
+ Coverage   54.98%   55.66%   +0.67%     
==========================================
  Files          19       19              
  Lines         902      918      +16     
==========================================
+ Hits          496      511      +15     
- Misses        354      355       +1     
  Partials       52       52
Impacted Files Coverage Δ
pkg/installation/install.go 38.98% <80%> (+1.81%) ⬆️
pkg/index/validation/validate.go 94.59% <89.47%> (+0.94%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 43b29c3...3e520e1. Read the comment docs.

the root of your plugin, use the default:

```yaml
files: [] # same as [{from: "*", to: "."}]

This comment has been minimized.

Copy link
@ahmetb

ahmetb Jul 16, 2019

Member

Hmm. I think empty list should not be treated as unspecified (nil) field.

Empty means do nothing, which a valid operation, but it shouldn't be allowed since it does not make sense.

This comment has been minimized.

Copy link
@corneliusweig

corneliusweig Jul 16, 2019

Author Contributor

Isn't the differentiation between empty and nil a programming language subtlety? From a user's perspective both cases mean "no rules given". And here I think we are complicating things if we make that distinction.

This comment has been minimized.

Copy link
@ahmetb

ahmetb Jul 16, 2019

Member

I'll move this to main thread.

files: [] # same as [{from: "*", to: "."}]
```

The resulting installation directory would eventually contain:

This comment has been minimized.

Copy link
@ahmetb

ahmetb Jul 16, 2019

Member

This would copy out binaries for both platforms to the installation directory on user’s machine, despite only one of them will be used.

@@ -90,8 +90,13 @@ func (p Platform) Validate() error {
if p.Bin == "" {
return errors.New("bin has to be set")
}
if len(p.Files) == 0 {
return errors.New("can't have a plugin without specifying file operations")
for _, fo := range p.Files {

This comment has been minimized.

Copy link
@ahmetb

ahmetb Jul 16, 2019

Member

Let's extract this to a new method that we can unit-test, just like ValidatePlatform above.

@@ -90,8 +90,13 @@ func (p Platform) Validate() error {
if p.Bin == "" {
return errors.New("bin has to be set")
}
if len(p.Files) == 0 {
return errors.New("can't have a plugin without specifying file operations")
for _, fo := range p.Files {

This comment has been minimized.

Copy link
@ahmetb

ahmetb Jul 16, 2019

Member

Let’s add a check that verifies nil is ok for p.Files (you should make this Files field a pointer like *[]FileOperaton first I think to distinguish between nil and empty []) AND validate that empty is not allowed and should not be defaulted.

return version, uri, p.Files, p.Bin, nil
files := p.Files
if len(files) == 0 {
files = []index.FileOperation{{From: "*", To: "."}}

This comment has been minimized.

Copy link
@ahmetb

ahmetb Jul 16, 2019

Member

worth adding a glog.V(4) here saying file operation not specified, assuming %v

wantBin: "kubectl-foo",
wantErr: false,
}, {
name: "Find Matching Platform with default files",

This comment has been minimized.

Copy link
@ahmetb

ahmetb Jul 16, 2019

Member

...with FileOperations unspecified or ...defaulted

@ahmetb

This comment has been minimized.

Copy link
Member

commented Jul 16, 2019

Overall looks good. I think we just need to distinguish clearly between unspecified (null) and empty ([]) while allowing one and failing on another.

@ahmetb

This comment has been minimized.

Copy link
Member

commented Jul 16, 2019

Isn't the differentiation between empty and nil a programming language subtlety?

Sadly, no. This is independent of programming language.
In one case, you're specifying "I have no elements" in another, it's unspecified altogether.

More obvious examples of this can be seen in Kubernetes API design. For example, in Kubernetes NetworkPolicy API, specifying ingress: [] as an empty array explicitly causes a policy to be created allowing no traffic. However, not specifying this field doesn't have that impact (it makes the NetworkPolicy object not an ingress policy at all).

Let's play safe in this case and (1) distinguish nil and default on that (2) validate error on empty array. Here's another reason to play safe: We can start with defaulting on just nil today and failijng on [], if it works out we can expand the definition. However, if we default on both nil and [] (and realize it's wrong API design), we can't easily change the behavior without breaking the API.

I prefer to err on being more restrictive and being more lenient only if there's a need.

@corneliusweig

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2019

Ok, that forward compatibility argument is convincing. I'll address the nil vs empty issue, but I'll need a few days due to other obligations.

@ahmetb

This comment has been minimized.

Copy link
Member

commented Jul 22, 2019

Haha sorry I caused merge conflicts everywhere. Now that we have proper validation on Platform, and testutil.NewPlugin() utilities, this PR should be easier to code.

@corneliusweig corneliusweig force-pushed the corneliusweig:w/optional-platform-spec branch from b9a58c3 to dfc1a2e Jul 22, 2019

@corneliusweig

This comment has been minimized.

Copy link
Contributor Author

commented Jul 22, 2019

/hold

@corneliusweig corneliusweig force-pushed the corneliusweig:w/optional-platform-spec branch from dfc1a2e to 946b1bf Jul 22, 2019

@corneliusweig

This comment has been minimized.

Copy link
Contributor Author

commented Jul 22, 2019

/hold cancel

@corneliusweig

This comment has been minimized.

Copy link
Contributor Author

commented Jul 22, 2019

Rebase is finally done.

I added a new validation case which forbids empty file operations now. The difference between empty and unspecified is quite subtle though (this comes from our yaml library):

      ...
      bin: foo-amd64
      files:  # present, but no values

is equivalent to

      ...
      bin: foo-amd64
#    files:  <- not present

Whereas this triggers the error case:

      ...
      bin: foo-amd64
      files: []
@ahmetb

This comment has been minimized.

Copy link
Member

commented Jul 22, 2019

I think it's ok to treat unspecified and files: (with no values) the same.
On the other hand, files: [] explicitly says I don't want any values in there.

└── krew-foo-windows.exe
```
- To copy all files in the `bin/` directory of the extracted archive to
the root of your plugin, use the default:

This comment has been minimized.

Copy link
@ahmetb

ahmetb Jul 22, 2019

Member

hmm, to use the default, maybe they shouldn't need to specify files:?

This comment has been minimized.

Copy link
@corneliusweig

corneliusweig Jul 23, 2019

Author Contributor

Well, my thinking was to keep the two examples as similar as possible. But I agree that this looks odd. I changed the wording.

}
for _, op := range fops {
if op.From == "" {
return errors.New("from field has to be set")

This comment has been minimized.

Copy link
@ahmetb

ahmetb Jul 22, 2019

Member

can we say "from" and "to" (below) to distinguish field names from the rest of the error message. especially below it says to ... to be set so it's a bit confusing.

This comment has been minimized.

Copy link
@corneliusweig

corneliusweig Jul 23, 2019

Author Contributor

I enclosed field names in backticks. To be consistent, I did the same for other error messages.

pkg/index/validation/validate.go Outdated Show resolved Hide resolved
@@ -190,8 +190,8 @@ func TestValidatePlatform(t *testing.T) {
wantErr: true,
},
{
name: "no file operations",
platform: testutil.NewPlatform().WithFiles(nil).V(),
name: "empty file operations",

This comment has been minimized.

Copy link
@ahmetb

ahmetb Jul 22, 2019

Member

can you also add WithFiles(nil) case (wantErr:false)?

This comment has been minimized.

Copy link
@corneliusweig

corneliusweig Jul 23, 2019

Author Contributor

Those test cases are covered by Test_validateFiles. I only included this error case in this test, to check if files are validated at all. Do you think this is not enough?

uri = p.URI
sha256sum = p.Sha256

files := p.Files
if len(files) == 0 {

This comment has been minimized.

Copy link
@ahmetb

ahmetb Jul 22, 2019

Member

I'm removing this method (getDownloadTarget) soon as part of #293.

I think we should consider a method like func applyDefaults(*index.Platform) to achieve defaulting on the FileOperations. Are you open to such a method, that's easily testable etc?

This comment has been minimized.

Copy link
@corneliusweig

corneliusweig Jul 23, 2019

Author Contributor

In principle, I like the idea. However, I didn't find a good spot for this. The best place is close to validation, but there we pass in a copy of the plugin object which loses any mutations. And I didn't want to put this into ReadPluginFile.

So I ended up, putting it in moveToInstallDir. Not ideal, though..

This comment has been minimized.

Copy link
@ahmetb

ahmetb Jul 23, 2019

Member

I think that's why we should call applyDefaults(&plugin) right before we do moveToInstallDir. Then we can do moveToInstallDir(..., plugin.Files).

Don't move inside moveToInstallDir as the name indicates, it's not expected to do defaulting.

This comment has been minimized.

Copy link
@corneliusweig

corneliusweig Jul 24, 2019

Author Contributor

Alright, I moved the logic one level up into install. Also, I added a unit test for applyDefaults.

Allow `files` specification to be unspecified in plugin manifests
If unspecified, default to `{From: "*", To: "."}`. When `files` is given,
but empty, an error is produced instead.

Also validate that files specification has both fields set.

@corneliusweig corneliusweig force-pushed the corneliusweig:w/optional-platform-spec branch from 946b1bf to e9e84d0 Jul 23, 2019

Review comments
- Improve readability of error messages.

@corneliusweig corneliusweig force-pushed the corneliusweig:w/optional-platform-spec branch from e9e84d0 to f357807 Jul 23, 2019

pkg/installation/move.go Outdated Show resolved Hide resolved
},
{
name: "no defaults for other fields",
platform: testutil.NewPlatform().WithBin("").WithOS("").WithSelector(nil).WithSHA256("").WithURI("").V(),

This comment has been minimized.

Copy link
@ahmetb

ahmetb Jul 24, 2019

Member

oh boy this test is gonna be a problem in the future.
because we will keep adding defaults to testutil.NewPlatform(), and we will keep adding new WithXxx.
let's keep it until it breaks.

@ahmetb

This comment has been minimized.

Copy link
Member

commented Jul 24, 2019

/lgtm
/approve

@k8s-ci-robot

This comment has been minimized.

Copy link

commented Jul 24, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahmetb, corneliusweig

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:
  • OWNERS [ahmetb,corneliusweig]

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 merged commit 8867a94 into kubernetes-sigs:master Jul 24, 2019

2 of 3 checks passed

tide Not mergeable. Needs lgtm label.
Details
cla/linuxfoundation corneliusweig authorized
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@corneliusweig corneliusweig deleted the corneliusweig:w/optional-platform-spec branch Jul 24, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.