Use special "detached" index for manifest plugins #568
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/XS
|
/area multi-index |
I think it’s fine. |
|
Since we introduce a behavior, we should test. Maybe add a test to integ tests of krewinstallmanifest ones loading receipt and checking source==detached. |
But technically we’re offering these libraries as part of our API. Would anyone build tooling/automation around this const? If not, better not expose? |
chriskim06
force-pushed
the
manifest-plugin-index
branch
from
496602f to
a8f4269
Compare
k8s-ci-robot
added
size/S
|
The integration test failed for some reason (worked locally), I think I vaguely remember seeing this in other PRs. I'll investigate further later tonight. |
Seems like it was a flake (somehow), re-running the workflow worked. It happened at least once before. I'm suspecting GitHub's endpoint sending us an error response somehow. Need to look at download code to see if it's checking for non-200 codes. |
integration_test/install_test.go
Outdated
| @@ -216,3 +220,14 @@ func localTestServer() (string, func()) { | |||
| s := httptest.NewServer(http.FileServer(http.Dir("testdata"))) | |||
| return s.URL, s.Close | |||
| } | |||
|
|
|||
| func assertManifestPluginIndex(t *testing.T, test *ITest, plugin string) { | |||
There was a problem hiding this comment.
assertPluginFromIndex is a better name potentially?
also why is this not defined on test (like assertExecutableInPath)? that way you don't have to pass it t.
There was a problem hiding this comment.
I can add it there. I added this to just check "detached" and thought that would be a little too specific to install --manifest to add to testutil_test.go.
integration_test/install_test.go
Outdated
| @@ -216,3 +220,14 @@ func localTestServer() (string, func()) { | |||
| s := httptest.NewServer(http.FileServer(http.Dir("testdata"))) | |||
| return s.URL, s.Close | |||
| } | |||
|
|
|||
| func assertManifestPluginIndex(t *testing.T, test *ITest, plugin string) { | |||
| receiptPath := test.TempDir().Path("receipts/" + plugin + constants.ManifestExtension) | |||
There was a problem hiding this comment.
don't guess filesystem layout; use environment.Paths (unless you're actually testing user-visible behavior, breaking behavior between versions, or Paths type).
|
@corneliusweig @ahmetb let me know what you guys think about "detached". There's now a few usages of it in the integration test so would a good compromise be to make it a public const in the cmd package so that its not exposed but still available within the code? Although I think that might be a little strange seeing |
|
@chriskim06 nothing wrong with actually duplicating those in integration_test. In fact, tests should rely on consts less and less. Because we want to test "behavior". If someone changes the value of the const, that should break the tests to notify us of behavior change, so please refrain from using the const in the tests. |
|
/lgtm Thanks! |
k8s-ci-robot
added
the
lgtm
|
[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 |
k8s-ci-robot
added
the
approved
Let me know if you want me to move this out to a package level variable, figured it would be ok like this since this situation is only encountered in this file.
Related issue: #483