-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor-x, closes #373 #386
Conversation
818d076
to
665ff50
Compare
please make sure to fix the tests and issues with code climate. |
6e3e2b9
to
3a755b6
Compare
x/xstrings/strings.go
Outdated
package xstrings | ||
|
||
// ContainsString returns true if slice s contains e element, false otherwise. | ||
func ContainsString(a []string, e string) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would be better rename to _ SliceContains()_
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ContainsString was chosen based on similarity to strings/bytes pkg:
Contains(s, substr string)
ContainsAny(s, chars string)
ContainsRune(s string, r rune)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see but in our case String word in the function name in xstrings package is not necessary because we define the package functions for the same context with package name. And for this case, it'd be good to indicate we're working on slices otherwise it looks like we're trying to find a substring in a string with current naming.
cmd/service/utils.go
Outdated
@@ -77,40 +75,17 @@ func downloadServiceIfNeeded(path string) (newPath string, didDownload bool, err | |||
if !govalidator.IsURL(path) { | |||
return path, false, nil | |||
} | |||
newPath, err = createTempFolder() | |||
newPath, err = ioutil.TempDir("", "mesg-") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we make mesg- a const
cmd/service/init.go
Outdated
@@ -140,12 +141,13 @@ func getTemplateResult(result string, templates []*templateStruct) (tmpl *templa | |||
} | |||
|
|||
func downloadTemplate(tmpl *templateStruct) (path string, err error) { | |||
path, err = createTempFolder() | |||
path, err = ioutil.TempDir("", "mesg-") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we make mesg- a const
cmd/service/init.go
Outdated
@@ -140,12 +141,13 @@ func getTemplateResult(result string, templates []*templateStruct) (tmpl *templa | |||
} | |||
|
|||
func downloadTemplate(tmpl *templateStruct) (path string, err error) { | |||
path, err = createTempFolder() | |||
path, err = ioutil.TempDir("", "mesg-") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we make mesg- a const
cmd/service/utils.go
Outdated
if err != nil { | ||
return "", false, err | ||
} | ||
if err := gitClone(path, newPath, "Downloading service..."); err != nil { | ||
fmt.Println("Downloading service...") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
forgot to use utils.ShowSpinnerForFunc()? it was defined in previous version of gitClone()
cmd/service/init.go
Outdated
if err != nil { | ||
return "", err | ||
} | ||
|
||
return path, gitClone(tmpl.URL, path, "Downloading template "+tmpl.Name+"...") | ||
fmt.Printf("Downloading template %s ...\n", tmpl.Name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
forgot to use utils.ShowSpinnerForFunc()? it was defined in previous version of gitClone()
cmd/service/utils_test.go
Outdated
@@ -20,33 +19,6 @@ func TestBuildDockerImagePathDoNotExist(t *testing.T) { | |||
require.NotNil(t, err) | |||
} | |||
|
|||
func TestGitCloneRepositoryDoNotExist(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should keep these tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed my mind, no need this here but got to put equivalent to xgit. and mock underlying git package if possible. we need these tests to see if we call git.PlainClone() with the right args.
cmd/service/utils_test.go
Outdated
require.NotNil(t, err) | ||
} | ||
|
||
func TestGitCloneWithoutURLSchema(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should keep these tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed my mind, no need this here but got to put equivalent to xgit. and mock underlying git package if possible. we need these tests to see if we call git.PlainClone() with the right args.
cmd/service/utils_test.go
Outdated
require.Nil(t, err) | ||
} | ||
|
||
func TestGitCloneCustomBranch(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should keep these tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed my mind, no need this here but got to put equivalent to xgit. and mock underlying git package if possible. we need these tests to see if we call git.PlainClone() with the right args.
94e6643
to
0ec576d
Compare
// | ||
// This function uses md5 hashing function and default formatter. See also Dump() | ||
// function. | ||
func Hash(c interface{}, version int) string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can test this with structhash.Version(hash) by comparing versions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope :P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't make sense to me, leaving this to @antho1404's watch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
x/xsignal/signal.go
Outdated
) | ||
|
||
// WaitForInterrupt creates a read channel for catch SIGINT and SIGTERM signals. | ||
func WaitForInterrupt() <-chan os.Signal { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can test this with:
- https://godoc.org/os#FindProcess
os.Getpid()
process.Signal(os.Interrupt)
andprocess.Signal(syscall.SIGTERM)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
leaving this to @antho1404's judgment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
x/xos/os.go
Outdated
if err != nil { | ||
return err | ||
} | ||
f.Close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can return error with an err
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
t.Fatalf("Touched file dosen't exist") | ||
} | ||
|
||
if err := Touch(name); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why we Touch twice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Touch creates a new file, truncating it if it already exists. => to make sure touch doesn't' return an error
name := filepath.Join(os.TempDir(), "__test-file") | ||
defer Remove(name) | ||
if err := Touch(name); err != nil { | ||
t.Fatalf("Touch failed: %s", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can use require package for testing x package
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I intentionally didn't use require package here to keep it as simple as possible and reduce dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't make sense to me, leaving this to @antho1404's watch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kind of prefer to use the require package in order to have a coherence in the codebase. And for now these packages are still in the project so it doesn't really matter to have this extra dependency but I understand that maybe one day it can be useful.
In other terms I don't mind one or the other are fine, it's not a big deal for me even if I would prefer to use require package from now to have consistency
x/xos/file.go
Outdated
return err | ||
} | ||
|
||
// RemoveAll removes all given paht and any children it contains. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
paht to path
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
x/xos/file.go
Outdated
} | ||
|
||
// RemoveAll removes all given paht and any children it contains. | ||
func RemoveAll(paths ...string) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can be renamed as RemoveRecursive, if you wish
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RemoveAll is fine for me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaving RemoveAll
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Except the few other remarks already in the PR it's good for me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you merge the dev branch please
cmd/service/init.go
Outdated
if err != nil { | ||
return "", err | ||
} | ||
|
||
return path, gitClone(tmpl.URL, path, "Downloading template "+tmpl.Name+"...") | ||
message := fmt.Sprintf("Downloading template %s ...\n", tmpl.Name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not valid, the \n
create some problems with the spinner. We should never use a \n
when it's a message for the spinner
⣾ Downloading template Javascript ...
⣽ Downloading template Javascript ...
⣻ Downloading template Javascript ...
⢿ Downloading template Javascript ...
⡿ Downloading template Javascript ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
x/xdocker/xarchive/archive.go
Outdated
@@ -0,0 +1,14 @@ | |||
package xrequire |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
xarchive
cmd/service/utils.go
Outdated
if err != nil { | ||
return err | ||
// prepareService downloads if needed, create the service, build it and inject configuration | ||
func prepareService(path string) *service.Service { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks to codeclimate, I think this function is not needed here, same for downloadServiceIfNeeded
or injectConfigurationInDependencies
because they moved to the mesg/deploy
@ilgooz can you confirm ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some changes to do, maybe related to a merge
) | ||
|
||
// GzippedTar creates an archive from the directory at path compressed with gzip. | ||
func GzippedTar(path string) (io.Reader, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is not used anywhere. Maybe a problem with the merge?
The codebase is still using TarWithOptions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i requested this one, will be used by tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not using it in the codebase instead of TarWithOptions
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes we should
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As @ilgooz pointed out, the function will be used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, please update the codebase to use this GzippedTar
function or at least create some issue to not forget. Otherwise, I worry that we will end up with a lot of function in x
packages that no other function.
Here are some place that use TarWithOptions
:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same with git clone, it's another branch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
) | ||
|
||
// Clone clones a repository from url into the path. | ||
func Clone(URL string, path string) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This following line is still using git.PlainClone
:
https://github.com/mesg-foundation/core/blob/8711a71037209288d39287805c7653f7bb043da9/mesg/deploy_deployer.go#L94
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, and? What's about the PlainClone?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not make the deploy_deployer
file use the function from x packages?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The following function has been moved to xgit:
So let's remove it, and update the code to use the one from xgit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but deploy_deplyer.go is inside another branch. So we need to merge this into the dev
and then merge dev into that branch and update code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
||
// GetenvDefault retrieves the value of the environment variable named by the key. | ||
// It returns the value, which will be set to fallback if the variable is empty. | ||
func GetenvDefault(key, fallback string) string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is never used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I would recommend to use Viper for the default value management
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left it if someone need this function (eg for testing)
#373