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

Fetch remote topo #1622

Closed
wants to merge 6 commits into from
Closed
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
19 changes: 18 additions & 1 deletion cmd/root.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,14 @@ package cmd

import (
"errors"
"fmt"
"os"
"path/filepath"
"time"

log "github.com/sirupsen/logrus"
"github.com/spf13/cobra"
"github.com/srl-labs/containerlab/utils"
)

var (
Expand Down Expand Up @@ -106,12 +108,27 @@ func getTopoFilePath(cmd *cobra.Command) error {
return nil
}

var err error
if utils.IsHttpUri(topo){
switch {
case utils.IsGitHubURL(topo):
err := utils.CheckSuffix(topo)
if err != nil {
return err
}
rawUrl := utils.GetRawURL(topo)
topo = utils.GetFileName(topo)
utils.DownloadFile(rawUrl, topo)
default:
return fmt.Errorf("unsupported git repositoy: %s", topo)
}
}

// if topo or name flags have been provided, don't try to derive the topo file
if topo != "" || name != "" {
return nil
}

var err error

log.Debugf("trying to find topology files automatically")

Expand Down
64 changes: 64 additions & 0 deletions utils/http.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
package utils

import (
"strings"
"errors"
"net/http"
"io"
"os"
)


func GetRawURL(url string) string {
if strings.Contains(url, "github.com") {
raw:=strings.Replace(url, "github.com", "raw.githubusercontent.com", 1)
return strings.Replace(raw, "/blob", "", 1)
}
return url
}

func IsGitHubURL(url string) bool {
return strings.Contains(url, "github")
Copy link
Collaborator

Choose a reason for hiding this comment

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

This contains in my view is too broad. since also a "raw.githubusercontent.com" url would match.
In cmd/root.go you use this in the switch and consecutively apply the GetRawUrl. Which then does the conversion.
although that would check for "github.com" it should probably be more kind of "consistent".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I'm grasping what you'd like to see from this method.
You mentioned that it would match on raw.githubusercontent.com and you're right. I had planned on it matching on either a generic github.com or the raw.github path to offer flexibility to the end user. It then proceeds to that GetRawURL method where it only converts it if its a generic github.com url else it returns the url without converting anything.
How would you envision this changing?


}

func GetFileName(url string) string {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should probably use https://pkg.go.dev/path#Base.

Copy link
Collaborator

Choose a reason for hiding this comment

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

actually this is also available under utils/file.go -> FilenameForURL
https://github.com/srl-labs/containerlab/blob/main/utils/file.go#L259

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated to use FilenameForURL.

split := strings.Split(url, "/")
return split[len(split)-1]
}

// required global variable for tests, otherwise comparison operator fails as error instances were not equal
var ErrInvalidSuffix = errors.New("valid URL passed in as topology file, but does not end with .yml or .yaml, endpoint must be an actual topology file")
func CheckSuffix(url string) error {
// check if topo ends with either .yml or .yaml
if !strings.HasSuffix(url, ".yml") && !strings.HasSuffix(url, ".yaml") {
return ErrInvalidSuffix
}
return nil
}

func DownloadFile(url string, ouputFileName string) error {
// Get the data
resp, err := http.Get(url)
if err != nil {
return err
}
if resp.StatusCode != 200 {
return errors.New("URL does not exist")
Copy link
Collaborator

@steiler steiler Oct 9, 2023

Choose a reason for hiding this comment

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

There are may other http status codes, that have different meanings, other then "URL does not exist".
The returned error should account for that.
Probably use https://pkg.go.dev/net/http#StatusText

Copy link
Collaborator

Choose a reason for hiding this comment

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

Generally I'm actually seeing that we already have the http download functions available.
Check out CopyFileContents https://github.com/srl-labs/containerlab/blob/main/utils/file.go#L86

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Valid point on the status code. As for the CopyFileConents, I tried to implement that originally but was not able to get it to work for the remote file contents. I'll re-explore and see if I can get to work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to use copy method.

}
defer resp.Body.Close()

// Create the file
out, err := os.Create(ouputFileName)
if err != nil {
return err
}
defer out.Close()

// Write the body to file
_, err = io.Copy(out, resp.Body)
if err != nil {
return err
}
return nil
}
130 changes: 130 additions & 0 deletions utils/http_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
package utils

import (
"net/http"
"net/http/httptest"
"os"
"testing"
)

func TestIsGithubURL(t *testing.T) {
// tests that github urls are detected
var tests = []struct {
input string
expected bool
}{
{"github.com", true},
{"github.com/containers/containerlab/blob/master/README.md", true},
{"google.com/containers", false},
{"google.com/containers/containerlab/blob/master/README.md", false},
{"gitlab.com/containers", false},
{"raw.githubusercontent.com/containers", true},
}
for _, test := range tests {
if output := IsGitHubURL(test.input); output != test.expected {
t.Error("Test Failed: {} inputted, {} expected, recieved: {}", test.input, test.expected, output)
}
}
}

func TestGetRawURL(t *testing.T) {
// tests that github urls are converted to raw urls evething else is left as is
var tests = []struct {
input string
expected string
}{
{"github.com", "raw.githubusercontent.com"},
{"github.com/containers/containerlab/blob/master/README.md", "raw.githubusercontent.com/containers/containerlab/master/README.md"},
{"google.com/containers", "google.com/containers"},
{"google.com/containers/containerlab/blob/master/README.md", "google.com/containers/containerlab/blob/master/README.md"},
{"gitlab.com/containers", "gitlab.com/containers"},
{"raw.githubusercontent.com/containers", "raw.githubusercontent.com/containers"},
}
for _, test := range tests {
if output := GetRawURL(test.input); output != test.expected {
t.Error("Test Failed: {} inputted, {} expected, recieved: {}", test.input, test.expected, output)
}
}
}

func TestGetFileName(t *testing.T) {
// tests for valid file name
var tests = []struct {
input string
expected string
}{
{"github.com", "github.com"},
{"github.com/containers/containerlab/blob/master/README.md", "README.md"},
{"google.com/containers", "containers"},
{"google.com/containers/containerlab/blob/master/README.md", "README.md"},
{"gitlab.com/containers", "containers"},
{"raw.githubusercontent.com/containers", "containers"},
}
for _, test := range tests {
if output := GetFileName(test.input); output != test.expected {
t.Error("Test Failed: {} inputted, {} expected, recieved: {}", test.input, test.expected, output)
}
}
}

func TestCheckSuffix(t *testing.T) {
// tests for valid suffix
var tests = []struct {
input string
expected error
}{
{"github.com", ErrInvalidSuffix},
{"github.com/containers/containerlab/blob/master/README.md", ErrInvalidSuffix},
{"google.com/containers", ErrInvalidSuffix},
{"google.com/containers/containerlab/blob/master/README.md", ErrInvalidSuffix},
{"gitlab.com/containers", ErrInvalidSuffix},
{"raw.githubusercontent.com/containers", ErrInvalidSuffix},
{"github.com/containers/containerlab/blob/master/README.yml", nil},
{"github.com/containers/containerlab/blob/master/README.yaml", nil},
}
for _, test := range tests {
if output := CheckSuffix(test.input); output != test.expected {
t.Error("Test Failed: {} inputted, {} expected, recieved: {}", test.input, test.expected, output)
}
}
}

func TestDownloadFile(t *testing.T) {
tempDir := os.TempDir()
// Create a mock HTTP server for testing
ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
// Respond with a sample file content for testing
if r.URL.Path == "/valid" {
w.WriteHeader(http.StatusOK)
w.Write([]byte("This is a sample file content"))
} else {
w.WriteHeader(http.StatusNotFound)
}
}))
defer ts.Close()

// Test case 1: Download a file that exists
outputFileName := tempDir + "/downloaded_file.txt"
err := DownloadFile(ts.URL+"/valid", outputFileName)
if err != nil {
t.Fatalf("Expected no error, but got: %v", err)
}

// Check the content of the downloaded file
content, err := os.ReadFile(outputFileName)
if err != nil {
t.Fatalf("Failed to read the downloaded file: %v", err)
}
expectedContent := "This is a sample file content"
if string(content) != expectedContent {
t.Errorf("Expected content: %s, but got: %s", expectedContent, string(content))
}
os.Remove(outputFileName)
// Test case 2: Download a file that does not exist (simulate a non-200 response)
outputFileName = tempDir + "/nonexistent_file.txt"
err = DownloadFile(ts.URL+"/nonexistent", outputFileName)
expectedErrorMsg := "URL does not exist"
if err == nil || err.Error() != expectedErrorMsg {
t.Errorf("Expected error message '%s', but got: %v", expectedErrorMsg, err)
}
}