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

Providing Repo Include Feature #1479

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 1 addition & 15 deletions pkg/kudoctl/cmd/repo_index.go
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ func (ri *repoIndexCmd) run() error {
if err != nil {
return err
}
merge(index, mergeIndex)
client.Merge(index, mergeIndex)
}

if err := index.WriteFile(ri.fs, target); err != nil {
Expand All @@ -150,20 +150,6 @@ func (ri *repoIndexCmd) run() error {
return nil
}

func merge(index *repo.IndexFile, mergeIndex *repo.IndexFile) {
// index is the master, any dups in the merged in index will have what is local replace those entries
for _, pvs := range mergeIndex.Entries {
for _, pv := range pvs {
err := index.AddPackageVersion(pv)
// this is most likely to be a duplicate pv, which we ignore (but will log at the right v)
if err != nil {
// todo: add verbose logging here
continue
}
}
}
}

func (ri *repoIndexCmd) mergeRepoConfig() (*repo.Configuration, error) {
if ri.mergeRepoName != "" {
return ri.repoConfig(ri.mergeRepoName)
Expand Down
12 changes: 11 additions & 1 deletion pkg/kudoctl/cmd/repo_index_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package cmd

import (
"bytes"
"fmt"
"io/ioutil"
"path/filepath"
"testing"
Expand Down Expand Up @@ -92,7 +93,16 @@ func TestRepoIndexCmd_MergeIndex(t *testing.T) {
mergeFile, _ := repo.ParseIndexFile(mergeBytes)

resultBuf := &bytes.Buffer{}
merge(indexFile, mergeFile)

p, err := filepath.Abs("testdata/include-index")
assert.NoError(t, err)
config := &repo.Configuration{
URL: fmt.Sprintf("file://%s", p),
}
client, err := repo.NewClient(config)
assert.NoError(t, err)

client.Merge(indexFile, mergeFile)
if err := indexFile.Write(resultBuf); err != nil {
t.Fatal(err)
}
Expand Down
1 change: 1 addition & 0 deletions pkg/kudoctl/util/repo/index.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ const defaultURL = "http://localhost/"
type IndexFile struct {
APIVersion string `json:"apiVersion"`
Entries map[string]PackageVersions `json:"entries"`
Includes []string `json:"includes,omitempty"`
Generated *time.Time `json:"generated"`
}

Expand Down
55 changes: 49 additions & 6 deletions pkg/kudoctl/util/repo/repo_operator.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (

"github.com/spf13/afero"

"github.com/kudobuilder/kudo/pkg/kudoctl/clog"
"github.com/kudobuilder/kudo/pkg/kudoctl/http"
"github.com/kudobuilder/kudo/pkg/kudoctl/kudohome"
)
Expand Down Expand Up @@ -50,20 +51,28 @@ func NewClient(conf *Configuration) (*Client, error) {

// DownloadIndexFile fetches the index file from a repository.
func (c *Client) DownloadIndexFile() (*IndexFile, error) {
var indexURL string
parsedURL, err := url.Parse(c.Config.URL)
if err != nil {
return nil, fmt.Errorf("parsing config url: %w", err)
}
parsedURL.Path = fmt.Sprintf("%s/index.yaml", strings.TrimSuffix(parsedURL.Path, "/"))

indexURL = parsedURL.String()
return c.downloadIndexFile(nil, parsedURL)
}

func (c *Client) downloadIndexFile(parent *IndexFile, url *url.URL) (*IndexFile, error) {
var resp *bytes.Buffer
if strings.HasPrefix(indexURL, "file:") {
b, _ := ioutil.ReadFile(parsedURL.Path)
var err error
// we need the index.yaml at the url provided
url.Path = fmt.Sprintf("%s/index.yaml", strings.TrimSuffix(url.Path, "/"))

if url.Scheme == "file" || strings.HasPrefix(url.String(), "file:") {
b, err := ioutil.ReadFile(url.Path)
if err != nil {
return nil, err
}
resp = bytes.NewBuffer(b)
} else {
resp, err = c.Client.Get(indexURL)
resp, err = c.Client.Get(url.String())
}
if err != nil {
return nil, fmt.Errorf("getting index url: %w", err)
Expand All @@ -75,5 +84,39 @@ func (c *Client) DownloadIndexFile() (*IndexFile, error) {
}

indexFile, err := ParseIndexFile(indexBytes)
//TODO (kensipe): err handling such that error in includes are ignored (but not root)
//TODO (kensipe): track which includes have happened so there are no repeats
for _, include := range indexFile.Includes {
iURL, err := url.Parse(include)
if err != nil {
clog.Printf("Unable to parse include url for %s", include)
}
nextIndex, err := c.downloadIndexFile(indexFile, iURL)
if err != nil {
return nil, err
kensipe marked this conversation as resolved.
Show resolved Hide resolved
}
if parent != nil {
c.Merge(parent, nextIndex)
} else {
c.Merge(indexFile, nextIndex)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this block works incorrectly for nested includes.

Assuming we have three repos that have nested includes: AA includes BB, BB includes CC.
BB and CC contain operator X.

The final index should contain BB.X, if I understand correctly.

With this code, we would:

  • downloadIndexFile(nil, urlAA) -> indexFile for AA has no operator X
  • downloadIndexFile(indexFileAA, urlBB)
  • downloadIndexFile(indexFileAA, urlCC)
  • c.Merge(indexFileAA, indexCC) -> indexFileAA now has indexCC.X operator
  • c.Merge(indexFileAA, indexBB) -> X operator is already in there and is skipped.

Either we:

  • c.Merge the current index file before we handle the includes
  • Not pass the parent IndexFile to downloadIndexFile and merge the returned file from that call

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IF AA -> BB and BB -> CC and BB and CC have X I would expect:

downloadIndexFile(nil, urlAA) -> indexFile for AA has no operator X
downloadIndexFile(indexFileAA, urlBB)
downloadIndexFile(indexFileBB, urlCC)
c.Merge(indexFileBB, indexCC) -> indexFileBB would ignore X from CC (it has one)
c.Merge(indexFileAA, indexBB) -> X is merged into AA from BB

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the case where

downloadIndexFile(indexFileAA, urlBB)
downloadIndexFile(indexFileAA, urlCC)

in your example would be true if AA included BB and CC

}

return indexFile, err
}

// Merge combines the Entries of 2 index files. The first index file is the master
// the second is merged into the first. Any duplicates are ignored.
func (c *Client) Merge(index *IndexFile, mergeIndex *IndexFile) {
// index is the master, any dups in the merged in index will have what is local replace those entries
for _, pvs := range mergeIndex.Entries {
for _, pv := range pvs {
err := index.AddPackageVersion(pv)
// this is most likely to be a duplicate pv, which we ignore (but will log at the right v)
if err != nil {
// todo: add verbose logging here
continue
}
}
}
}
26 changes: 26 additions & 0 deletions pkg/kudoctl/util/repo/repo_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,3 +42,29 @@ func TestLoadRepositories(t *testing.T) {
assert.Equal(t, r.CurrentConfiguration().Name, Default.Name)
assert.Equal(t, r.CurrentConfiguration().URL, Default.URL)
}

func TestDownloadMultiRepo(t *testing.T) {

p, err := filepath.Abs("testdata/include-index")
assert.NoError(t, err)
config := &Configuration{
URL: fmt.Sprintf("file://%s", p),
}
client, err := NewClient(config)
assert.NoError(t, err)
index, err := client.DownloadIndexFile()
assert.NoError(t, err)
// mysql package only there, if include worked
assert.NotNil(t, index.Entries["mysql"])

// the merge for flink will have 1 dup that doesn't merge
flink, err := index.FindFirstMatch("flink", "", "0.3.0")
assert.NoError(t, err)
assert.Equal(t, "correct flink", flink.Description)

// and a new version that does merge
flink, err = index.FindFirstMatch("flink", "", "0.4.0")
assert.NoError(t, err)
assert.Equal(t, "0.4.0", flink.OperatorVersion)

}
18 changes: 18 additions & 0 deletions pkg/kudoctl/util/repo/testdata/include-index/index.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
apiVersion: v1
entries:
flink:
- appVersion: 0.7.0
description: correct flink
digest: 0787a078e64c73064287751b833d63ca3d1d284b4f494ebf670443683d5b96dd
maintainers:
- email: <[email protected]>
name: Tom Runyon
- email: <[email protected]>
name: Ken Sipe
name: flink
operatorVersion: 0.3.0
urls:
- http://kudo.dev/flink
includes:
- ../included-repo
generated: "2020-04-19T15:04:00Z"
39 changes: 39 additions & 0 deletions pkg/kudoctl/util/repo/testdata/included-repo/index.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
apiVersion: v1
entries:
mysql:
- appVersion: "5.7"
digest: ad2451eaf68896490a2864afbb3fcd81e6fe3368e4bf001f8a23dc9cbcddf49a
maintainers:
- email: [email protected]
name: Nick Jones
name: mysql
operatorVersion: 0.2.0
urls:
- https://kudo-repository.storage.googleapis.com/0.10.0/mysql-5.7_0.2.0.tgz
flink:
- appVersion: 0.7.0
description: wrong flink (should NOT be included)
digest: 0787a078e64c73064287751b833d63ca3d1d284b4f494ebf670443683d5b96dd
maintainers:
- email: <[email protected]>
name: Tom Runyon
- email: <[email protected]>
name: Ken Sipe
name: flink
operatorVersion: 0.3.0
- appVersion: 0.8.0
description: this merges because the version doesn't exist in parent
digest: 0787a078e64c73064287751b833d63ca3d1d284b4f494ebf670443683d5b96dd
maintainers:
- email: <[email protected]>
name: Tom Runyon
- email: <[email protected]>
name: Ken Sipe
name: flink
operatorVersion: 0.4.0
urls:
- http://kudo.dev/flink

generated: "2020-04-19T15:04:00Z"