Uh oh!
There was an error while loading. Please reload this page.
copy from source repo - #31
Conversation
bigkevmcd
commented
Apr 1, 2020
@akihikokuroda looks like this needs a rebase for the debug functionality? |
bigkevmcd
left a comment
There was a problem hiding this comment.
I think it might be nicer to have a "Local" implementation of the Source interface.
All it needs to do is implement Walk, based on the local path, rather than hacking the Repository Walk to do double-duty.
| if err != nil { | ||
| return fmt.Errorf("failed to copy service: %w", err) | ||
| copied := []string{} | ||
| if fromSoruceRepo(fromURL) { |
| return fmt.Errorf("failed to copy service: %w", err) | ||
| copied := []string{} | ||
| if fromSoruceRepo(fromURL) { | ||
| source, err := s.repoFactory("https://source/sourceorg/sourcerepo", fromURL) |
There was a problem hiding this comment.
Maybe this should be "" rather than something that looks like a repo?
There was a problem hiding this comment.
Agreed, this line is confusing. What kinds of values does fromURL have in these cases? We expect the source to be a clone of a functional git repository in our primary use case but do we have to know the url it was cloned from? Is that what fromURL is?
There was a problem hiding this comment.
The fromURL is a directory path from the command line. The repoFactory needs a n url format string. I'll change the repoFactory to take "".
| return fmt.Errorf("failed to copy service: %w", err) | ||
| copied := []string{} | ||
| if fromSoruceRepo(fromURL) { | ||
| source, err := s.repoFactory("https://source/sourceorg/sourcerepo", fromURL) |
There was a problem hiding this comment.
Agreed, this line is confusing. What kinds of values does fromURL have in these cases? We expect the source to be a clone of a functional git repository in our primary use case but do we have to know the url it was cloned from? Is that what fromURL is?
| parsed.User = url.UserPassword("promotion", a.Token) | ||
| return parsed.String(), nil | ||
| } | ||
| func fromSoruceRepo(s string) bool { |
Uh oh!
There was an error while loading. Please reload this page.
| return err | ||
| } | ||
| func (r *Repository) Walk(base string, cb func(prefix, name string) error) error { |
There was a problem hiding this comment.
@bigkevmcd wrote,
I think it might be nicer to have a "Local" implementation of the Source interface.
All it needs to do is implement Walk, based on the local path, rather than hacking the Repository Walk to do double-duty.
If that avoided the need for passing local bool into Repository.Walk I'd be very happy. It's confusing to see method calls like
err := source.Walk(filePath, false, func(prefix, name string) error {
because it's not obvious what 'false' means in that context. Also,
repoBase = filepath.Join(r.LocalPath, "config")
Earlier in copy.go we saw,
// pathForDestServiceConfig defines where in a 'gitops' repository the local config file
// for a given service should live.
func pathForDestServiceConfig(serviceName, name string) string {
return filepath.Join("services/", serviceName, "base", name)
}
Even if you can refactor out a local Source.Walk() it would still be good to keep filepath manipulation in dedicated functions.
22a26b7 to
ea2e9cbCompareakihikokuroda
commented
Apr 1, 2020
I implemented the |
mnuttall
commented
Apr 2, 2020
That's a great improvement - thank you Aki :) |
This implements #19.
When --from is a local path, it copies
/path/to/local/repo/config/* tourl.to.target.repo/services/svc-name/base/config/*.