feat(service-registry artifact): allow loading artifact from remote URL#1345
feat(service-registry artifact): allow loading artifact from remote URL#1345rkpattnaik780 merged 2 commits intomainfrom
Conversation
|
I don't see any reviewer assigned for this or #1344 so not sure whether you would like them to be reviewed or not? |
craicoverflow
left a comment
craicoverflow
left a comment
There was a problem hiding this comment.
❯ ./rhoas service-registry artifact update --artifact-id foo_id --group DEFAULT --file "https://raw.githubusercontent.com/bolcom/avro-schema-viewer/master/docs/assets/avsc/1.0/schema.avsc"
Loading file from url: https://raw.githubusercontent.com/bolcom/avro-schema-viewer/master/docs/assets/avsc/1.0/schema.avsc
❌ 404 Not Found. Run the command in verbose mode using the -v flag to see more informationThis makes it seem like the file is not found, but in actual fact it is because the artifact does not exist. The user will not know that.
Overall this works really well 👍🏻 Not approving as I think there improvements that can be made but not outright rejecting it either. If you feel you want things kept as s that is fine, just re-request my review and let me know :)
pkg/cmdutil/cmdutil.go
Outdated
| // IsURL accepts a string and determines if it is a URL | ||
| func IsURL(s string) bool { | ||
| return strings.HasPrefix(s, "http:/") || strings.HasPrefix(s, "https:/") | ||
| } | ||
|
|
||
| // GetContentFromFileURL loads file content from the provided URL | ||
| func GetContentFromFileURL(url string, ctx context.Context) (*os.File, error) { | ||
|
|
||
| req, err := http.NewRequestWithContext(ctx, "GET", url, nil) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| client := http.DefaultClient | ||
|
|
||
| resp, err := client.Do(req) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if resp.StatusCode != http.StatusOK { | ||
| return nil, fmt.Errorf("error loading file: %s", http.StatusText(resp.StatusCode)) | ||
| } | ||
|
|
||
| respbody := resp.Body | ||
|
|
||
| defer resp.Body.Close() | ||
|
|
||
| tmpfile, err := ioutil.TempFile("", "rhoas_file-*") | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error initializing temporary file: %w", err) | ||
| } | ||
|
|
||
| defer func() { | ||
| _ = tmpfile.Close() | ||
| _ = os.Remove(tmpfile.Name()) | ||
| }() | ||
|
|
||
| _, err = io.Copy(tmpfile, respbody) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| specifiedFile, err := os.Open(tmpfile.Name()) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return specifiedFile, nil | ||
| } |
There was a problem hiding this comment.
cmdutil should be strictly for utils for creating commands. These functions are technically not tied to command creation and are generally very useful anywhere, I reckon a new/separate package might be good.
pkg/cmdutil/cmdutil.go
Outdated
| } | ||
|
|
||
| // GetContentFromFileURL loads file content from the provided URL | ||
| func GetContentFromFileURL(url string, ctx context.Context) (*os.File, error) { |
There was a problem hiding this comment.
nit: ctx usually goes first in functions
There was a problem hiding this comment.
Feels like a good candidate for a unit test.
pkg/cmdutil/cmdutil.go
Outdated
| } | ||
|
|
||
| if resp.StatusCode != http.StatusOK { | ||
| return nil, fmt.Errorf("error loading file: %s", http.StatusText(resp.StatusCode)) |
pkg/cmdutil/cmdutil.go
Outdated
|
|
||
| tmpfile, err := ioutil.TempFile("", "rhoas_file-*") | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error initializing temporary file: %w", err) |
There was a problem hiding this comment.
Should we use a struct for all the utility methods to pass common things like localizer, context, it seems bit odd to have an additional argument for everything?
There was a problem hiding this comment.
I agree, it also means we must continuously "break" APIs when new requirements are added such as i18n, which may not exist in a util but then if we need to add it we automatically break it..
A better thing to do might be to return a special error type like InitializeTemporaryFileErr and in the usage, interpret that and use i18n to create the message outside of the util..
Neither option seems ideal to me. Maybe merge it as it as its such a minor error, there's no point sacrificing usability/simplicity yet.
|
It works! Great job! |
|
This should be merged right? |
Will merge once approved. |
I see that @wtrocki has approved. Do PRs need 2 approvals? |
| @@ -1,9 +1,13 @@ | |||
| package util | |||
There was a problem hiding this comment.
I know it pre-exists this PR, but going forward we should not call packages "util", and we should update it when we see them.
| } | ||
|
|
||
| if resp.StatusCode != http.StatusOK { | ||
| return nil, fmt.Errorf("error loading file: %s", http.StatusText(resp.StatusCode)) |
|
|
||
| tmpfile, err := ioutil.TempFile("", "rhoas-std-input") | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error initializing temporary file: %w", err) |
3 participants
It should be possible to create/update artifacts using remote URLs.
Verification Steps
--filewhile creating an artifact.--fileto update an artifact.Type of change
Checklist