Fetch remote topo #1622
Fetch remote topo #1622
Conversation
friday963
commented
Oct 7, 2023
friday963
commented
- Adds functionality to pull in topology file from remote github repository.
- User can pass in topology file using generic https://github.com/path/to/yml/file.yml || https://raw.githubusercontent.com/path/to/yml/file.yaml.
- file can be either .yml or .yaml.
- Errors on any other url at this time.
|
Integration testing I conducted
|
utils/http.go
Outdated
| return err | ||
| } | ||
| if resp.StatusCode != 200 { | ||
| return errors.New("URL does not exist") |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Updated to use copy method.
utils/http.go
Outdated
|
|
||
| } | ||
|
|
||
| func GetFileName(url string) string { |
There was a problem hiding this comment.
This should probably use https://pkg.go.dev/path#Base.
There was a problem hiding this comment.
actually this is also available under utils/file.go -> FilenameForURL
https://github.com/srl-labs/containerlab/blob/main/utils/file.go#L259
There was a problem hiding this comment.
updated to use FilenameForURL.
| } | ||
|
|
||
| func IsGitHubURL(url string) bool { | ||
| return strings.Contains(url, "github") |
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
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?
|
@steiler my idea was that this should solve all artifacts retrieval, both topo file as well as all linked artifacts like license, binds, startup-configs, etc. |
|
yes, and e.g. the general license retrieval is already implemented in #1414 so they sould be merged and finalized altogether |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1622 +/- ##
==========================================
+ Coverage 48.55% 49.02% +0.46%
==========================================
Files 133 136 +3
Lines 12779 13136 +357
==========================================
+ Hits 6205 6440 +235
- Misses 5870 5985 +115
- Partials 704 711 +7
|
