Base git repository working #104
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
Hi @somtochiama. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
added
needs-ok-to-test
k8s-ci-robot
added
the
size/L
|
/assign @justinsb |
k8s-ci-robot
added
the
needs-rebase
| log.WithValues("path", name).Error(err, "error reading channel") | ||
| return nil, err | ||
| } | ||
| fmt.Println(string(b)) |
There was a problem hiding this comment.
Nit: this looks like leftover debug code. It's OK to log with a very high verbosity (e.g. 4 or 6) instead, but I would probably just delete it - it's most valuable when you're first developing/debugging.
|
|
||
| channel := &Channel{} | ||
| if err := yaml.Unmarshal(b, channel); err != nil { | ||
| return nil, fmt.Errorf("error parsing channel %s: %v", name, err) |
There was a problem hiding this comment.
(If you did want to log the bytes, this could be a good place to log it - when something has gone wrong. If we were debugging this, the first thing we would ask is "what is the contents of channel?")
|
|
||
| var filePath string | ||
| if r.subDir == "" { | ||
| filePath = fmt.Sprintf("packages/%v/%v/manifest.yaml", packageName, id) |
There was a problem hiding this comment.
If you use path.Join you can probably avoid the repetition here: https://play.golang.org/p/-XheAesp5eZ
(Note: path.Join and not filepath.Join, because even on windows we want a url-style path with forward slashes)
| if err != nil{ | ||
| return nil, err | ||
| } | ||
|
|
There was a problem hiding this comment.
Missing defer file.Close() I think.
I think you can also just do ioutil.ReadFile though, which is probably easier
| return nil, err | ||
| } | ||
|
|
||
| file, err := os.Open(repoDir + "/" + url) |
There was a problem hiding this comment.
Nit: using filepath.Join is generally easier & safer when constructing paths (and here, it's a filesystem path, so you want filepath, not path!)
|
|
||
| // checks for subdirectories | ||
| if strings.Contains(url, ".git//") { | ||
| newURL := strings.Split(url, ".git//") |
There was a problem hiding this comment.
Nit: if you use strings.SplitN(url, ".git//", 2) you will only split on the first .git//, and not drop any other segments.
An alternative would be to check that newURL ends up with len(.) == 2
(urlTokens or tokens or urlComponents or components might also be a more descriptive name, as it isn't really a URL)
|
As there's a conflict, a few last nits that it's worth looking at before we merge |
somtochiama
force-pushed
the
git-repository
branch
from
140096e to
f6bdae2
Compare
k8s-ci-robot
removed
the
needs-rebase
|
This looks great, thanks @somtochiama /approve |
k8s-ci-robot
added
ok-to-test
somtochiama
force-pushed
the
git-repository
branch
from
e86dad1 to
238d579
Compare
|
Thanks @somtochiama - tests are happy, and I'm happy :-) /approve |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: justinsb, SomtochiAma The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
No description provided.