re-adds source and docs for review #4
Conversation
| ``` | ||
| $ helm2bundle redis-1.1.12.tgz | ||
| $ ls | ||
| apb.yml Dockerfile redis-1.1.12.tgz |
There was a problem hiding this comment.
What do you think about following the same format as apb init and creating a directory named redis-1.1.12-apb? Or even dropping the version and redis-apb. I also know it might be dumb to create a directory of 2 files...
There was a problem hiding this comment.
Interesting idea. I think the main reason we wouldn't do that is that the image needs the chart file itself. All three of the files you see here have to be used while building the image. apb init is a bit different since it is starting from 0 assets.
There was a problem hiding this comment.
+1, doesn't really make sense the more I think about it.
helm2bundle.go
Outdated
| Async: "optional", | ||
| Metadata: map[string]string{ | ||
| "displayName": fmt.Sprintf("%s (helm bundle)", v.Name), | ||
| "console.openshift.io/iconClass": fmt.Sprintf("icon-%s", v.Name), // no guarantee it exists, but worth a shot |
There was a problem hiding this comment.
I'm assuming the worst that happens if it doesn't exist is some javascript errors?
There was a problem hiding this comment.
Not sure why imageUrl wouldn't work here. You would need to add the Chart's Icon as a field in TarValues and grab it from the Chart.yaml to use it. But that seems to work better.
There was a problem hiding this comment.
Yeah, good point. I need to give imageUrl another try here and will likely just switch to that. Not sure why it wasn't working for me previously, but I also didn't try very hard to troubleshoot it.
helm2bundle.go
Outdated
| Plans []Plan `yaml:"plans"` | ||
| } | ||
|
|
||
| type Plan struct { |
There was a problem hiding this comment.
Could vendor in broker-client-go to get these types. Might not be a bad idea so you don't have to maintain them.
helm2bundle.go
Outdated
| Async: "optional", | ||
| Metadata: map[string]string{ | ||
| "displayName": fmt.Sprintf("%s (helm bundle)", v.Name), | ||
| "console.openshift.io/iconClass": fmt.Sprintf("icon-%s", v.Name), // no guarantee it exists, but worth a shot |
There was a problem hiding this comment.
Not sure why imageUrl wouldn't work here. You would need to add the Chart's Icon as a field in TarValues and grab it from the Chart.yaml to use it. But that seems to work better.
| apb.yml Dockerfile redis-1.1.12.tgz | ||
| ``` | ||
|
|
||
| On OpenShift you can ``apb push`` to build and push the service bundle into your |
There was a problem hiding this comment.
typically you only need one back tick to format code inline. like this, no need to change in this document but could save you 2 extra characters in future documents. Imagine the time you could save in a lifetime :)
helm2bundle.go
Outdated
| Plans []Plan `yaml:"plans"` | ||
| } | ||
|
|
||
| type Plan struct { |
helm2bundle.go
Outdated
| Parameters []Parameter `yaml:"parameters"` | ||
| } | ||
|
|
||
| type Parameter struct { |
| "io/ioutil" | ||
| "os" | ||
| "path" | ||
| "text/template" |
There was a problem hiding this comment.
goimport tool will reorder the imports to be ordered as stdlib first and alphabetized. Then third party, alphabetized. I use vim-go with goimports which handles this for me.
import (
"archive/tar"
"compress/gzip"
"errors"
"fmt"
"io"
"io/ioutil"
"os"
"path"
"text/template"
"github.com/spf13/cobra"
"gopkg.in/yaml.v2"
)
| Use: "helm2bundle CHARTFILE", | ||
| Short: "Packages a helm chart as a Service Bundle", | ||
| Args: cobra.ExactArgs(1), | ||
| Run: func(cmd *cobra.Command, args []string) { |
There was a problem hiding this comment.
the func here is clever but makes the code harder to read since it is longer than a few lines. Not a blocker but I would've had a separate function for it. If we add new parameters I can see this main function becoming quite large.
There was a problem hiding this comment.
Agreed. I started just from the cobra examples, and it grew from there. I'll factor most of it out.
helm2bundle.go
Outdated
| } | ||
| } | ||
|
|
||
| // fileExists returns true if either apb.yml or Dockerfile exist in the working |
There was a problem hiding this comment.
I think it should read:
either apb.yml or Dockerfile exists in the working directory
|
|
||
| // getTarValues opens the helm chart tarball to 1) retrieve Chart.yaml so it can | ||
| // be parsed, and 2) retrieve the entire contents of values.yaml. | ||
| func getTarValues(filename string) (TarValues, error) { |
|
I think I've addressed everything except @rthallisey 's suggestion to vendor in the APB structs. I'm looking into that and may want to ask a couple of questions about it next week. |
|
@rthallisey I looked into vendoring in bundle-lib for the types, and I filed a bundle-lib issue about why that's not viable at the moment: automationbroker/bundle-lib#47 I also filed an issue to track it in this repo: #5 |
|
@mhrivnak I know it already got merged, but yes the refactor looks good. Thanks. |
|
Thanks @jmrodri |
I contrived these two branches to provide a review opportunity. Whatever revisions come out of it, I'll squash and cherry-pick onto master.