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

Add receipt status #526

Merged
merged 9 commits into from Mar 5, 2020
Merged

Conversation

@chriskim06
Copy link
Member

chriskim06 commented Feb 29, 2020

This adds the status object to the receipt type that was added earlier. The changes are pretty small, most of the additions are from tests implemented in the way outlined here.

Fixes #507
Related issue: #483

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Feb 29, 2020

Codecov Report

Merging #526 into master will increase coverage by 0.75%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #526      +/-   ##
==========================================
+ Coverage   58.73%   59.49%   +0.75%     
==========================================
  Files          23       23              
  Lines        1013     1022       +9     
==========================================
+ Hits          595      608      +13     
+ Misses        359      355       -4     
  Partials       59       59
Impacted Files Coverage Δ
internal/installation/receipt/receipt.go 88.23% <100%> (+13.23%) ⬆️
internal/index/indexscanner/scanner.go 72.88% <0%> (+6.77%) ⬆️

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 5c5c57b...fe00b8b. Read the comment docs.

uri: http://example.com/
shortDescription: in-memory test plugin object
version: v1.0.0-test.1
Status:

This comment has been minimized.

Copy link
@ahmetb

ahmetb Feb 29, 2020

Member

This shouldnt be parsing correctly, right?

This comment has been minimized.

Copy link
@chriskim06

chriskim06 Feb 29, 2020

Author Member

I didn't actually notice that I capitalized those. Let me look into it.

This comment has been minimized.

Copy link
@chriskim06

chriskim06 Feb 29, 2020

Author Member

I thought the json struct tags would affect how it was parsed but it doesn't seem to be that way. When the receipt is written it uses lower case letters for those keys. Am I missing something?

This comment has been minimized.

Copy link
@ahmetb

ahmetb Feb 29, 2020

Member

This just shouldn’t be parsing accurately.
We currently ignore unknown fields intentionally.

This comment has been minimized.

Copy link
@chriskim06

chriskim06 Mar 1, 2020

Author Member

I'm a little confused I think. What do you mean by unknown fields in this case? If you're referring specifically to the test receipt that has a custom index set in its status, I mainly added that even though only default can be set in a receipt because receipt.Load doesn't validate the index that is parsed. Let me know if I misunderstood.

This comment has been minimized.

Copy link
@ahmetb

ahmetb Mar 1, 2020

Member

All our fields are case-sensitive.
So as you can imagine, there can't be a field capitalized like Status.
So typing Status here should be the same as Asadklfjdksfj (which would be ignored).

This comment has been minimized.

Copy link
@chriskim06

chriskim06 Mar 2, 2020

Author Member

I tried changing version to Version in my receipts in my normal krew installation and was still able to parse receipts in kubectl krew list (where it loads the receipts and gets .Spec.Version from them). Maybe I'm doing something wrong. I also thought they were case sensitive and made a mistake by accidentally making those capitalized in the test file, but didn't notice it since the tests were passing and they were still parsing correctly.

This comment has been minimized.

Copy link
@ahmetb

ahmetb Mar 2, 2020

Member

That's very odd. Let's continue assuming they're case-sensitive.
We can investigate later why.

@@ -62,4 +62,17 @@ type FileOperation struct {
// Receipt describes a plugin receipt file.
type Receipt struct {
Plugin `json:",inline" yaml:",inline"`

Status ReceiptStatus `json:"status,omitempty"`

This comment has been minimized.

Copy link
@ahmetb

ahmetb Mar 1, 2020

Member

why ,omitempty? we want receipts to always have status (except today's receipts, which currently don't have, but once they are updated, they should).

This comment has been minimized.

Copy link
@chriskim06

chriskim06 Mar 2, 2020

Author Member

This was just me misremembering what we talked about before, I wrote up the receipt issue before you and I cleared that up and I copied over the struct definitions I wrote there and forgot to remove this. I'll get rid of it 👍

@ahmetb

This comment has been minimized.

Copy link
Member

ahmetb commented Mar 3, 2020

/lgtm
@corneliusweig for a quick review

Copy link
Contributor

corneliusweig left a comment

This already looks quite good. However, I'd like to reduce the number of extra test files. Ideally, you don't even need to add extra yaml files at all (see below).
But if you have a good reason to add these, please remove one of them because it does not add anything.

}
tests := []struct {
name string
args args

This comment has been minimized.

Copy link
@corneliusweig

corneliusweig Mar 3, 2020

Contributor

I haven't read the previous review comments, but is there a reason why receiptFilePath is wrapped in this args struct? There could as well just be a single receiptFilePath field.

This comment has been minimized.

Copy link
@chriskim06

chriskim06 Mar 3, 2020

Author Member

Its left over from a previous version. I had some other things there before that I moved out, I'll make the change.

internal/index/indexscanner/scanner_test.go Outdated Show resolved Hide resolved
wantStatus: index.ReceiptStatus{},
},
{
name: "read receipt of plugin from default index with status",

This comment has been minimized.

Copy link
@corneliusweig

corneliusweig Mar 3, 2020

Contributor
Suggested change
name: "read receipt of plugin from default index with status",
name: "read receipt with explicit source index",
dest := tmpDir.Path("some-plugin.yaml")

if err := Store(testPlugin, dest); err != nil {
t.Fatal(err)
}

actual, err := indexscanner.LoadPluginByName(tmpDir.Root(), "some-plugin")
actual, err := indexscanner.ReadReceiptFromFile(tmpDir.Path("some-plugin.yaml"))

This comment has been minimized.

Copy link
@corneliusweig

corneliusweig Mar 3, 2020

Contributor
Suggested change
actual, err := indexscanner.ReadReceiptFromFile(tmpDir.Path("some-plugin.yaml"))
actual, err := indexscanner.ReadReceiptFromFile(dest)
args: args{
receiptFilePath: filepath.Join(testdataPath(t), "testindex", "receipts", "foo.yaml"),
},
Comment on lines 108 to 110

This comment has been minimized.

Copy link
@corneliusweig

corneliusweig Mar 3, 2020

Contributor

You could also use your testdata builder NewReceipt().WithXX().V() and store that in a temporary directory with tmpDir.WriteYAML(...).
Why did you opt for test fixtures instead?

This comment has been minimized.

Copy link
@chriskim06

chriskim06 Mar 3, 2020

Author Member

Ya I can change it to this. I think I did it this way because this is how it was outlined in the previous receipts PR and I just had that on my mind while I was working on this initially.

This comment has been minimized.

Copy link
@corneliusweig

corneliusweig Mar 4, 2020

Contributor

Yeah, we talked about both options. Back then we didn't have the receipt builder, so the static file was easier. But now, I think the dynamic approach is far better.

@ahmetb

This comment has been minimized.

Copy link
Member

ahmetb commented Mar 3, 2020

It's also currently unclear to me where we should be doing the "defaulting" of the missing status.index.name. Deferring this to the caller is error-prone. Potentially parsing function could do that.

WDYT @corneliusweig?

@k8s-ci-robot k8s-ci-robot removed the lgtm label Mar 3, 2020
@corneliusweig

This comment has been minimized.

Copy link
Contributor

corneliusweig commented Mar 4, 2020

Doing the defaulting while reading leads to the least duplication, so let's do it that way. Good idea!

@k8s-ci-robot k8s-ci-robot added size/M and removed size/L labels Mar 4, 2020
chriskim06 added 5 commits Feb 29, 2020
Add tests to scanner_test.go to verify reading receipt files
@chriskim06 chriskim06 force-pushed the chriskim06:receipt-status branch from c1f485e to 22764a6 Mar 4, 2020
Copy link
Contributor

corneliusweig left a comment

The last two little nits, then it's ready to merge from my side.

internal/index/indexscanner/scanner.go Show resolved Hide resolved
pkg/index/types.go Show resolved Hide resolved
@ahmetb

This comment has been minimized.

Copy link
Member

ahmetb commented Mar 4, 2020

/approve
we're close1!!!1

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Mar 4, 2020

[APPROVALNOTIFIER] This PR is APPROVED

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

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 size/L and removed size/M labels Mar 4, 2020
wantStatus := &index.ReceiptStatus{
Source: index.SourceIndex{
Name: name,
type args struct {

This comment has been minimized.

Copy link
@ahmetb

ahmetb Mar 5, 2020

Member

for this time it's ok but in the future I recommend avoiding single-field type args

This comment has been minimized.

Copy link
@chriskim06

chriskim06 Mar 5, 2020

Author Member

Crap sorry about that, you mentioned that before. I'll remember next time and change this in a follow up PR most likely.

@ahmetb

This comment has been minimized.

Copy link
Member

ahmetb commented Mar 5, 2020

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Mar 5, 2020
@k8s-ci-robot k8s-ci-robot merged commit 31e0f49 into kubernetes-sigs:master Mar 5, 2020
7 of 8 checks passed
7 of 8 checks passed
build
Details
tide Not mergeable.
Details
Header rules No header rules processed
Details
Pages changed 17 new files uploaded
Details
Redirect rules No redirect rules processed
Details
Mixed content No mixed content detected
Details
cla/linuxfoundation chriskim06 authorized
Details
deploy/netlify Deploy preview ready!
Details
@corneliusweig

This comment has been minimized.

Copy link
Contributor

corneliusweig commented Mar 5, 2020

🎊 One more step! So nice to see this going forward!

@chriskim06 chriskim06 mentioned this pull request Mar 28, 2020
6 of 6 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants
You can’t perform that action at this time.