Update plugin listings to use install receipts #276
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
approved
- Changed the way we determine the list of "installed plugins" by looking at install receipts directory. - Added a receipt.Load() method for reading saved install receipts. Signed-off-by: Ahmet Alp Balkan <[email protected]>
ahmetb
force-pushed
the
receipt-based-list
branch
from
a31ae7d to
83e9cff
Compare
Codecov Report
@@ Coverage Diff @@
## master #276 +/- ##
==========================================
+ Coverage 53.94% 55.34% +1.39%
==========================================
Files 18 19 +1
Lines 862 880 +18
==========================================
+ Hits 465 487 +22
+ Misses 341 337 -4
Partials 56 56
Continue to review full report at Codecov.
|
| plugins, err := ioutil.ReadDir(installDir) | ||
| // ListInstalledPlugins returns a list of all install plugins in a | ||
| // name:version format based on the install receipts at the specified dir. | ||
| func ListInstalledPlugins(receiptsDir string) (map[string]string, error) { |
There was a problem hiding this comment.
Nice, that just got a lot simpler 👍
Would be a good time to also add some tests for this.
pkg/receipt/receipt_test.go
Outdated
| @@ -68,3 +70,20 @@ func TestStore(t *testing.T) { | |||
| t.Fatal(diff) | |||
| } | |||
| } | |||
|
|
|||
| func TestLoad(t *testing.T) { | |||
| p, err := Load(filepath.Join("..", "..", "integration_test", "testdata", "foo.yaml")) | |||
There was a problem hiding this comment.
This doesn't feel right. Depending on test data in another package looks like headache in the future.
What about writing the plugin manifest to disk and read with Load? For that it could be helpful to have a testutil.ValidPluginManifest global value which can be used by other tests as well.
There was a problem hiding this comment.
What about writing the plugin manifest to disk and read with
Load?
this is a good idea.
For that it could be helpful to have a
testutil.ValidPluginManifestglobal value
this is mentioned in #270
There was a problem hiding this comment.
Hmm let's keep it as is, I'll add a TODO here. I have two concerns:
- I don't want to do minor: duplicate test object cleanup #270 in this PR.
- Ideally I don't want Load()’s tests to fail because of a fault in Store(). But I think we can live with it.
Signed-off-by: Ahmet Alp Balkan <[email protected]>
|
Looks great! |
k8s-ci-robot
added
the
lgtm
|
[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:
Approvers can indicate their approval by writing |
install receipts directory.
/assign @corneliusweig