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 a new receipt type #507

Closed
chriskim06 opened this issue Feb 21, 2020 · 4 comments · Fixed by #526
Closed

Add a new receipt type #507

chriskim06 opened this issue Feb 21, 2020 · 4 comments · Fixed by #526

Comments

@chriskim06
Copy link
Member

chriskim06 commented Feb 21, 2020

Plugin receipts currently use the index.Plugin type. In order to support the changes to support multiple indexes outlined in #483, it was decided that the information for which index a plugin is installed from would be stored in the receipt. In order to support this, a new receipt type should be added. Here's a pretty simple proposed structure of what a receipt type might look like:

// Receipt describes a receipt manifest file.
type Receipt struct {
        Plugin   `json:",inline" yaml:",inline"`

        Status ReceiptStatus `json:"status,omitempty"`
}

// ReceiptStatus contains information about the installed plugin.
type ReceiptStatus struct {
        Source SourceIndex `json:"source"`
}

// SourceIndex contains information about the index a plugin was installed from.
type SourceIndex struct {
        // Name is the configured name of an index a plugin was installed from.
        Name string `json:"name"`
}
@corneliusweig
Copy link
Contributor

corneliusweig commented Feb 22, 2020

We probably also want omitempty.

I think it's best to start with a minimal version for the Receipt type. So

type Receipt Plugin

should be enough for the initial migration.


After the migration is done, we can add fields as required. I think we should only add those fields which are necessary. At the moment we only have a use-case for SourceIndex, so what you suggest looks great.
However, we need to think about the semantics of that field. Is it the tap name at the time of install (which might change)? Or maybe the git remote url (which should be more static)? This btw should go into the GoDoc for that field.

@chriskim06
Copy link
Member Author

Agreed, that sounds like the best way to introduce the receipt type at first.

I don't really have a strong opinion on what SourceIndex should be. I think the index name will probably be easier to work with from a code perspective but I don't know off the top of my head how we would deal with a user adding a different index with the same name and having krew not try to update a plugin installed from the deleted index with the same name. Does that use case fall under the category of edge cases we agreed would have intentionally undefined behavior?

@ahmetb
Copy link
Member

ahmetb commented Feb 22, 2020

+1 design looks good. However it’s not extensible. I recommend this:

  • status:
    • sourceIndex:
      • name:

If the name-based soln doesn’t work out long-term we can migrate much easier this way.

We’d probably need a ReceiptFromPlugin method to convert plugin to a receipt in a central place ideally.

@chriskim06
Copy link
Member Author

Makes sense, I'll update the design.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants