ship onprem pulls web assets from url #114
Conversation
kevherro
changed the title
kevherro
changed the title
![codeclimate[bot]](https://avatars.githubusercontent.com/in/9403?s=60&v=4){kind=small}
| @@ -53,6 +53,10 @@ func (p *CLIPlanner) Build(assets []api.Asset, configGroups []libyaml.ConfigGrou | |||
| asset.DockerLayer.Dest = filepath.Join("installer", asset.DockerLayer.Dest) | |||
| debug.Log("event", "asset.resolve", "asset.type", "dockerlayer") | |||
| plan = append(plan, p.dockerLayerStep(*asset.DockerLayer, meta)) | |||
| } else if asset.Web != nil { | |||
There was a problem hiding this comment.
Avoid deeply nested control flow statements.
There was a problem hiding this comment.
Let's not argue with the robots now. You might start a war.
| "github.com/replicatedhq/ship/pkg/templates" | ||
| ) | ||
|
|
||
| func (p *CLIPlanner) webStep(web *api.WebAsset, configGroups []libyaml.ConfigGroup, meta api.ReleaseMetadata, templateContext map[string]interface{}) Step { |
There was a problem hiding this comment.
Method CLIPlanner.webStep has a Cognitive Complexity of 27 (exceeds 20 allowed). Consider refactoring.
| "github.com/replicatedhq/ship/pkg/templates" | ||
| ) | ||
|
|
||
| func (p *CLIPlanner) webStep(web *api.WebAsset, configGroups []libyaml.ConfigGroup, meta api.ReleaseMetadata, templateContext map[string]interface{}) Step { |
There was a problem hiding this comment.
Method CLIPlanner.webStep has 67 lines of code (exceeds 50 allowed). Consider refactoring.
| }, | ||
| ) | ||
|
|
||
| builtUrl, err := builder.String(web.URL) |
There was a problem hiding this comment.
var builtUrl should be builtURL
pkg/lifecycle/render/web/step.go
Outdated
| } | ||
| } | ||
|
|
||
| func (p *DefaultStep) Execute( |
There was a problem hiding this comment.
Method DefaultStep.Execute has a Cognitive Complexity of 29 (exceeds 20 allowed). Consider refactoring.
pkg/lifecycle/render/web/step.go
Outdated
| } | ||
| } | ||
|
|
||
| func (p *DefaultStep) Execute( |
There was a problem hiding this comment.
Method DefaultStep.Execute has 68 lines of code (exceeds 50 allowed). Consider refactoring.
| RegisterResponders func() | ||
| } | ||
|
|
||
| func TestWebStep(t *testing.T) { |
There was a problem hiding this comment.
Function TestWebStep has 116 lines of code (exceeds 50 allowed). Consider refactoring.
| RegisterResponders func() | ||
| } | ||
|
|
||
| func TestWebStep(t *testing.T) { |
There was a problem hiding this comment.
Function TestWebStep has 5 return statements (exceeds 4 allowed).
| } | ||
| } | ||
|
|
||
| func (p *DefaultStep) buildAsset( |
There was a problem hiding this comment.
Method DefaultStep.buildAsset has 7 return statements (exceeds 4 allowed).
| RegisterResponders func() | ||
| } | ||
|
|
||
| func TestWebStep(t *testing.T) { |
There was a problem hiding this comment.
Function TestWebStep has 8 return statements (exceeds 4 allowed).
| Method: "POST", | ||
| URL: "http://foo.bar", | ||
| }, | ||
| RegisterResponders: func() { |
There was a problem hiding this comment.
Similar blocks of code found in 2 locations. Consider refactoring.
| Method: "POST", | ||
| URL: "http://foo.bar", | ||
| }, | ||
| RegisterResponders: func() { |
There was a problem hiding this comment.
Similar blocks of code found in 2 locations. Consider refactoring.
| }, | ||
| ExpectedErr: nil, | ||
| }, | ||
| { |
There was a problem hiding this comment.
Similar blocks of code found in 2 locations. Consider refactoring.
| RegisterResponders func() | ||
| } | ||
|
|
||
| func TestWebStep(t *testing.T) { |
There was a problem hiding this comment.
Function TestWebStep has a Cognitive Complexity of 23 (exceeds 20 allowed). Consider refactoring.
| Method: "POST", | ||
| URL: "http://foo.bar", | ||
| }, | ||
| RegisterResponders: func() { |
There was a problem hiding this comment.
Similar blocks of code found in 2 locations. Consider refactoring.
|
|
||
| func TestWebStep(t *testing.T) { | ||
| tests := []TestWebAsset{ | ||
| { |
There was a problem hiding this comment.
Similar blocks of code found in 2 locations. Consider refactoring.
| }, | ||
| ExpectedErr: nil, | ||
| }, | ||
| { |
There was a problem hiding this comment.
Similar blocks of code found in 2 locations. Consider refactoring.
| ExpectFiles: map[string]interface{}{}, | ||
| ExpectedErr: errors.New("Get web asset from http://foo.bar: received response with status 500"), | ||
| }, | ||
| { |
There was a problem hiding this comment.
Similar blocks of code found in 2 locations. Consider refactoring.
pkg/lifecycle/render/web/step.go
Outdated
| configGroups []libyaml.ConfigGroup, | ||
| templateContext map[string]interface{}, | ||
| ) (*Built, error) { | ||
| configCtx, err := templates.NewConfigContext(p.Logger, configGroups, templateContext) |
There was a problem hiding this comment.
is there a reason this isn't using p.BuilerBuilder to get the config context?
pkg/lifecycle/render/web/step.go
Outdated
| } | ||
|
|
||
| func parseRequest(url string, method string, body string) (*http.Request, error) { | ||
| switch method { |
There was a problem hiding this comment.
What about other methods like PUT
There was a problem hiding this comment.
I think there is a way to simplify this -- the switch blocks are almost the same
There was a problem hiding this comment.
i'm going to stick my guns on this one, but i'm happy to discuss irl. different methods need different resources (i.e., a POST needs a jsonified body, while a GET needs a nil body). i'd like to sacrifice compactness for modularity here and a switch on the method type accomplishes this
pkg/lifecycle/render/web/step.go
Outdated
| return nil, errors.Errorf("received response with status %d", resp.StatusCode) | ||
| } | ||
|
|
||
| defer resp.Body.Close() |
pkg/lifecycle/render/web/step.go
Outdated
|
|
||
| mode := os.FileMode(0644) | ||
| if asset.Mode != os.FileMode(0) { | ||
| debug.Log("event", "applying override permissions") |
There was a problem hiding this comment.
this log line should log the mode we're overriding with
pkg/lifecycle/render/web/step.go
Outdated
| mode = asset.Mode | ||
| } | ||
|
|
||
| bodyToBytes, byteErr := ioutil.ReadAll(body.Body) |
There was a problem hiding this comment.
@divolgin would know better than me, but it seems sub-optimal to read the whole asset into memory, and then write it to the filesystem -- I think go readers/writers are designed so you can stream data from a source like an http response down to the filesystem without loading into memory.
For example, what happens if someone wants to pull a large (>1GB) file here?
There was a problem hiding this comment.
This could be someone downloading large asset files. We should totally stream responses directly to disk.
kevherro
changed the title
|
Code Climate has analyzed commit 8795ef5 and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 68.7% (50% is the threshold). This pull request will bring the total coverage in the repository to 49.5% (0.8% change). View more on Code Climate. |
kevherro
changed the title
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| if resp.StatusCode > 299 { |
There was a problem hiding this comment.
Should we follow redirects and also handle 204 status?
There was a problem hiding this comment.
i would have thought Go HTTP would follow redirects but good question. I can't think of a use case for 204 but I also can't think of a reason to treat it as a failure.
There was a problem hiding this comment.
from the go docs:
If CheckRedirect is nil, the Client uses its default policy, which is to stop after 10 consecutive requests.
as for handling 204s ... i think a log would suffice. that way the user could see a 204 occurred and would have some context to accompany the blank file it produced
What I Did
Web Assets! You can now use the content of HTTP requests as content in your app.
How I Did it
I created a
WebAssetAsset type that specifies the required parameters to perform the Asset creation (what URL to pull from, where to store the raw HTML content, themethodtype, etc.). It works much like the other Assets - it is considered a Step that will be added to the Planner.How to verify it
.../web/step_test.goDescription for the Changelog
Picture of a Boat (not required but encouraged)