feat(update): #23 add fossa update command #36

xizhao · 2018-02-20T23:12:13Z

Uses github.com/rhysd/go-github-selfupdate/selfupdate to update binary.

Not tested yet, do not merge.

elldritch

Preliminary nits.

elldritch · 2018-02-20T23:23:06Z

build/sbt.go

@@ -144,10 +143,10 @@ func (builder *SBTBuilder) IsBuilt(m module.Module, allowUnresolved bool) (bool,

 // IsModule is not implemented
 func (builder *SBTBuilder) IsModule(target string) (bool, error) {
-	return false, errors.New("IsModule is not implemented for SBTBuilder")
+	return false, fmt.Errorf("IsModule is not implemented for SBTBuilder")


You don't need to use fmt.Errorf if you don't need the format string.

This still needs to be fixed. Also below, in the same file.

elldritch · 2018-02-20T23:24:02Z

cmd/fossa/update.go

+var updateLogger = logging.MustGetLogger("update")
+var updateEndpoint := "fossas/fossa-cli"
+
+func checkUpdate() error {


This should return (bool, error). Don't use error to represent things that are not errors.

elldritch · 2018-02-20T23:25:29Z

cmd/fossa/update.go

+
+func updateCmd(c *cli.Context) {
+	if err := checkUpdate(); err != nil {
+		updateLogger.Fatalf("unable to update (%s)", err)


Logged messages (distinct from error messages) should generally be Sentence case. The reason why error messages should not be Sentence case is because they're usually embedded within a sentence when they're finally logged to display to the user (e.g. logger.Fatalf("Problem happened: %s", err.Error())).

elldritch · 2018-02-20T23:25:57Z

cmd/fossa/update.go

+
+func updateCmd(c *cli.Context) {
+	if err := checkUpdate(); err != nil {
+		updateLogger.Fatalf("unable to update (%s)", err)


err is of type error. You probably mean err.Error(), which is of type string.

elldritch · 2018-02-20T23:26:34Z

cmd/fossa/update.go

+
+func updateCmd(c *cli.Context) {
+	if err := checkUpdate(); err != nil {
+		updateLogger.Fatalf("unable to update (%s)", err)


Why the wrapping parentheses? Error messages should always be readable as conventional strings. If you need to display the value of the error itself, you should use %#v.

elldritch

Small changes needed, but generally LGTM.

elldritch · 2018-02-21T00:28:44Z

build/sbt.go

@@ -144,10 +143,10 @@ func (builder *SBTBuilder) IsBuilt(m module.Module, allowUnresolved bool) (bool,

 // IsModule is not implemented
 func (builder *SBTBuilder) IsModule(target string) (bool, error) {
-	return false, errors.New("IsModule is not implemented for SBTBuilder")
+	return false, fmt.Errorf("IsModule is not implemented for SBTBuilder")


This still needs to be fixed. Also below, in the same file.

elldritch · 2018-02-21T00:29:35Z

cmd/fossa/main.go

@@ -244,6 +252,10 @@ func defaultCmd(c *cli.Context) {
 		mainLogger.Fatalf("Could not load configuration: %s", err.Error())
 	}

+	if ok, err := checkUpdate(); ok && err == nil {


[nit] Would prefer err == nil && ok. Generally, error checking goes first.

elldritch · 2018-02-21T00:31:49Z

cmd/fossa/main.go

@@ -244,6 +252,10 @@ func defaultCmd(c *cli.Context) {
 		mainLogger.Fatalf("Could not load configuration: %s", err.Error())
 	}

+	if ok, err := checkUpdate(); ok && err == nil {
+		mainLogger.Infof("An update is available for this CLI; run `fossa update` to get the latest version.")


This won't actually log anything for most users. See logging levels from go-logging:

const ( CRITICAL Level = iota ERROR WARNING NOTICE INFO DEBUG )

We currently only log WARNING and above by default (see initialize in config.go). I'm alright with changing the default logging level to INFO and above, although I'd actually consider this a NOTICE.

elldritch · 2018-02-21T00:33:46Z

cmd/fossa/update.go

+func checkUpdate() (bool, error) {
+	latest, found, err := selfupdate.DetectLatest(updateEndpoint)
+	if err != nil {
+		return false, err // unable to update due to GH error


You should annotate this error with fmt.Errorf. For example, return false, fmt.Errorf("could not detect latest comment: %s", err.Error()).

elldritch · 2018-02-21T00:37:15Z

cmd/fossa/update.go

+	}
+	latest, err := selfupdate.UpdateSelf(v, updateEndpoint)
+	if err != nil {
+		return err


Use fmt.Errorf to annotate this error. It helps us trace errors through the stack.

elldritch · 2018-02-23T06:24:21Z

I'm merging this. I know it works, but our build is breaking because of backend issues causing our license check to fail.

xizhao requested a review from elldritch February 20, 2018 23:12

elldritch reviewed Feb 20, 2018

View reviewed changes

elldritch approved these changes Feb 21, 2018

View reviewed changes

xizhao and others added 7 commits February 22, 2018 17:59

feat(update): #23 add fossa update command

2e9af5d

feat(update): add semver parsing

5d76012

feat(update): add debug logging for update

9e570f9

chore(update): fix nits

95e446e

feat(update): represent states better in update cmd

7ce1571

fix(update): fix update default logic

765cbcd

style: fix PR nits

7393553

elldritch force-pushed the feat/selfupdate branch 2 times, most recently from 65c24e9 to 23620d9 Compare February 23, 2018 02:33

ci: Improve logging and diagnostics on upload failure

29d496b

elldritch force-pushed the feat/selfupdate branch from 23620d9 to 29d496b Compare February 23, 2018 02:35

ci: Improve CI caching

546d381

elldritch force-pushed the feat/selfupdate branch from f342aa3 to 546d381 Compare February 23, 2018 03:07

fix(http): Always close HTTP request connections

6300fa4

elldritch force-pushed the feat/selfupdate branch from 65e0fde to 6300fa4 Compare February 23, 2018 03:59

elldritch added 2 commits February 22, 2018 21:19

fix(http): Correctly handle EOF timeouts

d577588

fix(http): Improve timeout handling

da90056

elldritch merged commit ec4e164 into master Feb 23, 2018

elldritch deleted the feat/selfupdate branch March 1, 2018 20:17

meghfossa pushed a commit that referenced this pull request Nov 12, 2021

configure --enable-executable-static in ci (#36)

c4a2df9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(update): #23 add fossa update command #36

feat(update): #23 add fossa update command #36

xizhao commented Feb 20, 2018 •

edited

Loading

elldritch left a comment

elldritch Feb 20, 2018

elldritch Feb 21, 2018

elldritch Feb 20, 2018

elldritch Feb 20, 2018

elldritch Feb 20, 2018

elldritch Feb 20, 2018

elldritch left a comment

elldritch Feb 21, 2018

elldritch Feb 21, 2018

elldritch Feb 21, 2018

elldritch Feb 21, 2018

elldritch Feb 21, 2018

elldritch commented Feb 23, 2018

feat(update): #23 add fossa update command #36

feat(update): #23 add fossa update command #36

Conversation

xizhao commented Feb 20, 2018 • edited Loading

elldritch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elldritch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elldritch commented Feb 23, 2018

xizhao commented Feb 20, 2018 •

edited

Loading