Code review and refactor

CODE_REVIEW.md

@@ -0,0 +1,48 @@
+# Notes
+Here are my notes in reviewing/refactoring this code:
+
+1. Idiomatic Go is a thing. Embrace it.
+
+2. All in all, your code was really, really good.  My code review/refactoring is taking you from 80% to 90%, with some of my review/refactors are based on my personal style that AFAIK does not conflict with idiomatic Go but really helps with readability and maintainability.  _(I can't take you to 100% because I'm just not that good; I doubt almost anyone is.)_
+
+3. One of the first things I think developers new to Go do is see packages and immediately start envisioning how their namespaces will allow logical code organization within an applications. And then these developers start creating a lot of packages with generic names. I absolutely did that. Unfortunately however I found through painful outcomes that using packages to organize code within an application is not a smart approach. Generic names often conflict with packages names in the Go standard library, and it is very easy to create cyclical dependencies across your packages which then requires you to work very hard to create internal interfaces you would otherwise never need just to get your over-organized code to compile. In reality your packages are rarely actually independent because one so often cannot really standalone without the functionality of the others. 
+
+4. You had a lot of code in your commands that could really be re-envisioned as methods for your structs which really simplify the code in your commands and allows for reuse of functionality now or down the road. 
+
+5. So for any given application or library package it is better — ignoring the required `main` package — to stick with one, or a very small number of packages, where the `main()` func is in the `main()` package — if you are writing an application — and the rest of code should be in a library named after your application, e.g. `lpm` in your case.  I reorganized your code in my fork to this effect.       
+
+6. Your two main files (`darwin.go` and `windows.go`) were almost exactly the same except for one expression difference. I refactored that difference into `utils` and I made your `main.go` file simple and easy to grok.
+
+7. Your functions should _(almost)_ always return an error in Go when an error occurs that your func cannot handle itself. Just failing to terminal is not a good strategy. You will also be able to add a lot more information so you user can figure out what happened. And if you ever want to use the same packages for microservices you will thank me.
+
+8. So when possible, if you want to fail on an error then bubble up that error while wrapping it with more info then handle at the root, in a CLI. Don't just fail deep in the middle of code because you may later want to reuse that code and if you do it won't be robust and will require lots of rearchitecture.
+
+9. When using text to describe error messages start with lower case so multiple messages can be composed without it looking weird. Also don't use periods to end error messages.
+
+10. Don't create an empty instance to be able to call a method. Just define as a func. That's idiomatic in Go and acceptable. If for want an instance for dependency injection but still want to call it explictly elsewhere, create both a method and a func that creates an instance to be able to call the method. So don't write `util.HTTPUtil{}.Get()`. Just write `util.HttpGet()`
+
+11. I refactored out `else{}` statements. [Embrace the happy-path](https://medium.com/@matryer/line-of-sight-in-code-186dd7cdea88). 
+
+12. It's more idiomatic in Go to use fmt.Sprintf() than string concatenation.
+
+13. My personal style is to use `goto end` rather than early `return` because it provides numerous benefits. Unfortunately in Go it also requires all declarations be above the `goto`. But then explict declarations help the reader of your code, so I don't think that's really a bad thing. Also doing it this way causes the developer to have to work harder to write longer methods, so this approach encourages more modularity.
+
+14. `errors` is a well-known package name in Go; you probably should not use it for your own package name.
+
+15. For cross-platform paths, use `filepath.FromSlash()` and `filepath.ToSlash()`
+
+16. I use GoLand from JetBrains as my editor/IDE _(which I cannot praise too much.)_ Thus I added `//goland:noinspection` in a few places because in those places it flagged that it reasonably thought were errors that were not really errors, such as not capturing the return value of a `.Close()` in a `defer` statement.
+
+17. In `Dependencies` you use pointer and non-pointer receivers (`d`) in your methods. Better to be consistent and use the same. I know it has less immutability benefit to use pointers, but mixing can cause people who have to use your packages to struggle. If you need immutability code for it explicitly, such as how `append()` works in Go.
+
+18. You can define types such as 'ClasspathFiles' so you can type `[]fs.FileInfo` in just one place and so that when you use them only values that are meant to be used for Classpath Files can be used for Classpath files, vs. some other slice of files.
+
+19. Your configuration in App variables makes it harder to write unit tests, from experience. Every times I have started with variables for config I ultimately end up having to refactor to using a struct with methods.
+
+20. Things that run within `func init()` should not be of the nature to throw errors. It will make debugging edge cases much harder. Do things that might throw errors in the main flow of your program.
+
+21. It appears that `Dependency` is a `map[string]string` but it looks like it never has more than a single key and a single value. Why not a simple struct? 
+
+22. Test in the same package has too much access to internals. Better to create a separate `test` package
+
+23. In general I have tried to move away from calculating things in init() functions and instead do so on demand in methods, e.g. see `Context.GetManifestFilepath()` vs. `app.FileLocation`. 

app/lpm/main.go

@@ -0,0 +1,17 @@
+package main
+
+import (
+	_ "embed" // Embed Import for Package Files
+	"os"
+	"package-manager/pkg/lpm"
+	"package-manager/pkg/lpm/cmd"
+)
+
+func main() {
+
+	err := cmd.Execute("/")
+	if err != nil {
+		lpm.ShowUserError(err)
+		os.Exit(1)
+	}
+}

app/populator/module.go

@@ -0,0 +1,219 @@
+package main
+
+import (
+	"fmt"
+	"github.com/gocolly/colly/v2"
+	"github.com/hashicorp/go-version"
+	"package-manager/pkg/lpm"
+	"sort"
+	"strings"
+)
+
+type Module struct {
+	name          string
+	category      string
+	url           string
+	includeSuffix string
+	excludeSuffix string
+	filePrefix    string
+}
+
+func (m Module) GetJarPath(tag string) string {
+	return fmt.Sprintf("%s/%s/%s%s.jar",
+		m.url,
+		tag,
+		m.filePrefix,
+		tag)
+}
+
+func (m Module) GetSha1Path(tag string) string {
+	return fmt.Sprintf("%s.sha1", m.GetJarPath(tag))
+}
+
+func (m Module) getCheckSum(tag string, algo lpm.ChecksumAlgorithm) (cs string, err error) {
+	var url string
+	var _sha []byte
+	var sha string
+
+	switch algo {
+	case lpm.Sha1Algorithm:
+		url = m.GetSha1Path(tag)
+	default:
+		err = fmt.Errorf("invalid checksum algorithm '%s'", string(algo))
+		goto end
+	}
+	_sha, err = lpm.HttpGet(url)
+	if err != nil {
+		err = fmt.Errorf("unable to retrieve %s: %w", url, err)
+		goto end
+	}
+	sha = string(_sha)
+	if strings.Contains(sha, "html") {
+		sha = ""
+	}
+	if 40 > len(sha) {
+		goto end
+	}
+	cs = sha[0:40] //Get first 40 character of SHA1 only
+
+end:
+	return cs, err
+}
+
+func (m Module) onAHref(f *colly.HTMLElement, vs []string) []string {
+	var text string
+
+	if strings.Contains(f.Text, "../") {
+		goto end
+	}
+
+	if strings.Contains(f.Text, "maven-metadata.") {
+		goto end
+	}
+
+	switch true {
+
+	case m.HasNoSuffixes():
+		text = strings.TrimSuffix(f.Text, "/")
+
+	case m.HasExcludeSuffix():
+		if m.ContainsExcludeSuffix(f.Text) {
+			goto end
+		}
+		text = strings.TrimSuffix(f.Text, "/")
+
+	case m.HasIncludeSuffix():
+		if !m.ContainsIncludeSuffix(f.Text) {
+			goto end
+		}
+		text = strings.TrimSuffix(f.Text, m.includeSuffix+"/")
+
+	case m.HasBothSuffixes():
+		if m.ContainsExcludeSuffix(f.Text) {
+			goto end
+		}
+		if !m.ContainsIncludeSuffix(f.Text) {
+			goto end
+		}
+		text = strings.TrimSuffix(f.Text, m.includeSuffix+"/")
+
+	}
+
+	vs = append(vs, text)
+end:
+	return vs
+}
+
+func (m Module) ContainsIncludeSuffix(s string) bool {
+	// @TODO Using Contains below seems like they could matching
+	//       false positives. Regex would probably better, but I
+	//       do not know the exact format to look for.
+	return strings.Contains(s, m.includeSuffix)
+}
+func (m Module) ContainsExcludeSuffix(s string) bool {
+	// @TODO Using Contains below seems like they could matching
+	//       false positives. Regex would probably better, but I
+	//       do not know the exact format to look for.
+	return strings.Contains(s, m.excludeSuffix)
+}
+func (m Module) HasNoSuffixes() bool {
+	return m.excludeSuffix == "" && m.includeSuffix != ""
+}
+func (m Module) HasExcludeSuffix() bool {
+	return m.excludeSuffix != "" && m.includeSuffix == ""
+}
+func (m Module) HasIncludeSuffix() bool {
+	return m.includeSuffix != "" && m.excludeSuffix == ""
+}
+func (m Module) HasBothSuffixes() bool {
+	return m.excludeSuffix != "" && m.includeSuffix != ""
+}
+
+func (m Module) retrieveVersionsViaHttp() (versions []*version.Version, err error) {
+	var versionsRaw []string
+
+	// Get Versions from Root package site
+	c := colly.NewCollector()
+	// Find and visit all links
+	c.OnHTML("a[href]", func(f *colly.HTMLElement) {
+		versionsRaw = m.onAHref(f, versionsRaw)
+	})
+
+	err = c.Visit(m.url)
+	if err != nil {
+		err = fmt.Errorf("unable to visit '%s': %w", m.url, err)
+		goto end
+	}
+
+	// Make Sorted Versions
+	versions = make([]*version.Version, len(versionsRaw))
+	for i, raw := range versionsRaw {
+		v, _ := version.NewVersion(raw)
+		versions[i] = v
+	}
+	sort.Sort(version.Collection(versions))
+
+end:
+	return versions, err
+
+}
+
+func (m Module) getNewVersion(p lpm.Package, tag string) (ver lpm.Version, err error) {
+
+	ver.Tag = tag
+
+	if m.includeSuffix != "" {
+		tag += m.includeSuffix
+	}
+
+	if m.filePrefix == "" {
+		m.filePrefix = p.Name
+	}
+
+	ver.Path = m.GetJarPath(tag)
+
+	ver.Algorithm = lpm.Sha1Algorithm
+	ver.CheckSum, err = m.getCheckSum(tag, ver.Algorithm)
+
+	return ver, err
+}
+
+func (m Module) getNewVersions(p lpm.Package) (lpm.Package, error) {
+	var vs []*version.Version
+	var ver lpm.Version
+	var err error
+
+	// Retrieve the version by URL
+	vs, err = m.retrieveVersionsViaHttp()
+	if err != nil {
+		err = fmt.Errorf("unable to retrieve vs for package '%s': %w",
+			p.Name,
+			err)
+		goto end
+	}
+
+	//Look for new versions
+	for _, v := range vs {
+		tag := v.Original()
+
+		pv := p.GetVersion(tag)
+
+		if pv.Tag != "" {
+			// if remote version is already in package manifest skip it
+			continue
+		}
+
+		ver, err = m.getNewVersion(p, v.Original())
+
+		if err != nil {
+			// Older vs might have bad version patterns
+			// ending up with a missing sha. Don't add them.
+			continue
+		}
+
+		p.Versions = append(p.Versions, ver)
+
+	}
+end:
+	return p, err
+}

app/populator/modules.go

@@ -0,0 +1,58 @@
+package main
+
+var modules Modules
+
+type Modules []Module
+
+func (es Modules) getByName(n string) Module {
+	var m Module
+	for _, e := range es {
+		if e.name == n {
+			m = e
+			break
+		}
+	}
+	return m
+}
+
+func init() {
+	modules = []Module{
+		{"postgresql", "driver", "https://repo1.maven.org/maven2/org/postgresql/postgresql", "", ".jre", ""},
+		{"mssql", "driver", "https://repo1.maven.org/maven2/com/microsoft/sqlserver/mssql-jdbc", ".jre11", ".jre11-preview", "mssql-jdbc-"},
+		{"mariadb", "driver", "https://repo1.maven.org/maven2/org/mariadb/jdbc/mariadb-java-client", "", "", "mariadb-java-client-"},
+		{"h2", "driver", "https://repo1.maven.org/maven2/com/h2database/h2", "", "", ""},
+		{"db2", "driver", "https://repo1.maven.org/maven2/com/ibm/db2/jcc", "", "db2", "jcc-"},
+		{"snowflake", "driver", "https://repo1.maven.org/maven2/net/snowflake/snowflake-jdbc", "", "", "snowflake-jdbc-"},
+		{"sybase", "driver", "https://repo1.maven.org/maven2/net/sf/squirrel-sql/plugins/sybase", "", "", ""},
+		{"firebird", "driver", "https://repo1.maven.org/maven2/net/sf/squirrel-sql/plugins/firebird", "", "", ""},
+		{"sqlite", "driver", "https://repo1.maven.org/maven2/org/xerial/sqlite-jdbc", "", "", "sqlite-jdbc-"},
+		{"oracle", "driver", "https://repo1.maven.org/maven2/com/oracle/ojdbc/ojdbc8", "", "", "ojdbc8-"},
+		{"mysql", "driver", "https://repo1.maven.org/maven2/mysql/mysql-connector-java", "", "", "mysql-connector-java-"},
+		{"liquibase-cache", "extension", "https://repo1.maven.org/maven2/org/liquibase/ext/liquibase-cache", "", "", ""},
+		{"liquibase-cassandra", "extension", "https://repo1.maven.org/maven2/org/liquibase/ext/liquibase-cassandra", "", "", ""},
+		{"liquibase-cosmosdb", "extension", "https://repo1.maven.org/maven2/org/liquibase/ext/liquibase-cosmosdb", "", "", ""},
+		{"liquibase-db2i", "extension", "https://repo1.maven.org/maven2/org/liquibase/ext/liquibase-db2i", "", "", ""},
+		{"liquibase-filechangelog", "extension", "https://repo1.maven.org/maven2/org/liquibase/ext/liquibase-filechangelog", "", "", ""},
+		{"liquibase-hanadb", "extension", "https://repo1.maven.org/maven2/org/liquibase/ext/liquibase-hanadb", "", "", ""},
+		{"liquibase-hibernate5", "extension", "https://repo1.maven.org/maven2/org/liquibase/ext/liquibase-hibernate5", "", "", ""},
+		{"liquibase-maxdb", "extension", "https://repo1.maven.org/maven2/org/liquibase/ext/liquibase-maxdb", "", "", ""},
+		{"liquibase-modify-column", "extension", "https://repo1.maven.org/maven2/org/liquibase/ext/liquibase-modify-column", "", "", ""},
+		{"liquibase-mongodb", "extension", "https://repo1.maven.org/maven2/org/liquibase/ext/liquibase-mongodb", "", "", ""},
+		{"liquibase-mssql", "extension", "https://repo1.maven.org/maven2/org/liquibase/ext/liquibase-mssql", "", "", ""},
+		{"liquibase-neo4j", "extension", "https://repo1.maven.org/maven2/org/liquibase/ext/liquibase-neo4j", "", "", ""},
+		{"liquibase-oracle", "extension", "https://repo1.maven.org/maven2/org/liquibase/ext/liquibase-oracle", "", "", ""},
+		{"liquibase-percona", "extension", "https://repo1.maven.org/maven2/org/liquibase/ext/liquibase-percona", "", "", ""},
+		{"liquibase-postgresql", "extension", "https://repo1.maven.org/maven2/org/liquibase/ext/liquibase-postgresql", "", "", ""},
+		{"liquibase-redshift", "extension", "https://repo1.maven.org/maven2/org/liquibase/ext/liquibase-redshift", "", "", ""},
+		{"liquibase-snowflake", "extension", "https://repo1.maven.org/maven2/org/liquibase/ext/liquibase-snowflake", "", "", ""},
+		{"liquibase-sqlfire", "extension", "https://repo1.maven.org/maven2/org/liquibase/ext/liquibase-sqlfire", "", "", ""},
+		{"liquibase-teradata", "extension", "https://repo1.maven.org/maven2/org/liquibase/ext/liquibase-teradata", "", "", ""},
+		{"liquibase-verticaDatabase", "extension", "https://repo1.maven.org/maven2/org/liquibase/ext/liquibase-verticaDatabase", "", "", ""},
+		//"liquibase-compat",
+		//"liquibase-javalogger",
+		//"liquibase-nochangeloglock",
+		//"liquibase-nochangelogupdate",
+		//"liquibase-sequencetable",
+		//"liquibase-vertica",
+	}
+}

app/populator/populator.go

@@ -0,0 +1,50 @@
+package main
+
+import (
+	"fmt"
+	"os"
+	"package-manager/pkg/lpm"
+)
+
+func main() {
+
+	var newPacks = make(lpm.Packages, 0)
+	var pack lpm.Package
+	var err error
+
+	// @TODO Verify '/' is the right thing here...
+	cfg := lpm.NewContext("/")
+	err = cfg.Initialize()
+
+	// read packages from embedded file
+	packs, err := cfg.UnmarshalPackages(lpm.PackagesJSON)
+	if err != nil {
+		goto end
+	}
+	for _, p := range packs {
+		m := modules.getByName(p.Name)
+		if m.name == "" {
+			newPacks = append(newPacks, p)
+			continue
+		}
+		pack, err = m.getNewVersions(p)
+		if err != nil {
+			err = fmt.Errorf("failed to get versions for %s: %w",
+				m.name,
+				err)
+			goto end
+		}
+		// Get new versions for a package
+		newPacks = append(newPacks, pack)
+	}
+
+	//WriteManifest all packages back to manifest.
+	err = cfg.WritePackages(newPacks)
+
+end:
+	if err != nil {
+		fmt.Printf("ERROR %s\n", err.Error())
+		os.Exit(1)
+	}
+
+}