Automation support #10

nikooo777 · 2018-04-30T19:28:04Z

This addresses https://github.com/lbryio/internal-issues/issues/165

tzarebczan · 2018-05-02T16:41:51Z

@nikooo777 Is this ready for review?

nikooo777 · 2018-05-02T21:30:10Z

negative Tom :) it's work in progress

nikooo777 · 2018-05-17T22:49:25Z

@lyoshenka there is a lot of work in here, It's not yet ready to be merged but if you want you can take a look at it already and let me know what I should change I'm happy with it.

There is some code reuse that I'd like to address and logging is inconsistent: some goes to slack and some doesn't.

lyoshenka

overall these changes look very messy. I know my ytsync code was messy to begin with, so I'm partially to blame as well. but now that we're planning to use the code for a while, we should come up with a coherent structure and move towards cleaner code. i can discuss with you if you want.

lyoshenka · 2018-05-23T13:42:06Z

cmd/selfsync.go

+//PoC
+func fetchChannels(authToken string, status string) ([]APIYoutubeChannel, error) {
+	url := "https://api.lbry.io/yt/jobs"
+	payload := strings.NewReader("------WebKitFormBoundary7MA4YWxkTrZu0gW\r\n" +


what is this?

the other solution we discussed worked too :)

lyoshenka · 2018-05-23T13:43:23Z

cmd/selfsync.go

+
+//PoC
+func fetchChannels(authToken string, status string) ([]APIYoutubeChannel, error) {
+	url := "https://api.lbry.io/yt/jobs"


don't hardcode this url. the API domain and endpoints should be configured somewhere

lyoshenka · 2018-05-23T13:46:06Z

util/disk_space.go

@@ -0,0 +1,18 @@
+package util


please avoid having a util package as much as possible. it becomes a dump for random code. this function is only used in the cmd package, so it can just be declared there

lyoshenka · 2018-05-23T13:47:23Z

util/slack.go

@@ -0,0 +1,49 @@
+package util


i think this is gonna be in beamer's PR, but it should not go into util either. we should make our own slack package.

lyoshenka · 2018-05-23T13:47:57Z

util/slack_test.go

+	"testing"
+)
+
+func TestSendToSlack(t *testing.T) {


this doesn't test anything

lyoshenka · 2018-05-23T13:57:26Z

ytsync/ytsync.go


 func (s *Sync) doSync() error {
 	var err error

 	err = s.walletSetup()
 	if err != nil {
-		return err
+		return errors.Err("Initial wallet setup failed! Manual Intervention is required. Reason: %v", err)


use errors.Prefix so you don't lose the original trace

lyoshenka · 2018-05-23T14:02:51Z

util/slack.go

+		if err != nil {
+			host = "???"
+		}
+		_, _, err = slackApi.PostMessage(slackChannel, message, slack.PostMessageParameters{Username: "YTSYNC (" + host + ")"})


lbry.go is a general package used in lots of other places. you can't have a general util function called sendToSlack that says YTSYNC in the message. all ytsync-specific stuff should go into the ytsync package

lyoshenka · 2018-05-23T14:04:02Z

cmd/selfsync_test.go

+// such field should be reset to null if the test must be run on a different machine (different hostname)
+// and obviously the auth token must be appropriate
+func TestSetChannelSyncStatus(t *testing.T) {
+	err := setChannelSyncStatus("620280", "UCNQfQvFMPnInwsU_iGYArJQ", StatusSyncing)


are you making calls to the live API here? this is a very weak test. and you're including the auth_token in public code that's visible on github?

just to make this clear. the first parameter is the auth token, which is just a number as I'm using this "test" locally. The second parameter is a channel ID.

I basically wrote tests to run the portion of code without having to compile and run the whole program, I believe more code should be tested in a better way. I don't think any of the tests here actually test anything.

Also yesh, I'm calling a local API, not the live API, but the right approach would be to mock the API.

lyoshenka · 2018-05-23T14:06:45Z

cmd/selfsync.go

@@ -0,0 +1,234 @@
+package cmd


keep the cmd package very small. it should just gather the CLI options and call a function in the ytsync package. lbry.go is shared across many different projects. keep your changes in ytsync package as much as possible.

lyoshenka · 2018-05-23T14:07:33Z

cmd/common.go

@@ -0,0 +1,22 @@
+package cmd
+
+const defaultMaxTries = 3


this is not common to cmd. this is common to ytsync. you also don't need a separate common.go file. just put it in an existing file

add failure cases change interrupt behavior for fatal errors bump up timeout to 20 minutes change max file size to 2GB remove unnecessary utxo wait

improve failure handling adjust slack logging increase file size in string increase publish timeout to 40 minutes

merge sync and selfsync export sync manager out of cmd package clean up ytsync.go address #19 and #20

move functions out of utils address review comments revert lockfile changes

lyoshenka · 2018-07-19T14:37:46Z

cmd/selfsync.go

+)
+
+func init() {
+	var selfSyncCmd = &cobra.Command{


why the switch from ytsync to selfsync. This package is called lbry.go, so its very general. selfsync is too generic a name.

you're right. I had this built when ytsync was still there, that's why it had a different name. I will change it back.

lyoshenka · 2018-07-19T14:41:38Z

util/slack.go

@@ -46,6 +47,26 @@ func SendToSlack(message string) error {
 	return sendToSlack(defaultChannel, defaultUsername, message)
 }

+// SendErrorToSlack Sends an error message to the default channel and to the process log.
+func SendErrorToSlack(format string, a ...interface{}) error {


these functions should not be in slack.go. first of all, they log the error twice, since sendToSlack already calls log.Debugln. second, all they do is add an emoji. you can put these functions someplace in the ytsync package if they are used a lot there, but they should not be out here.

however, using fmt.Sprintf is a nice touch. i support adding that to the existing Send functions.

lyoshenka · 2018-07-19T14:45:23Z

util/slice.go

@@ -8,3 +10,13 @@ func InSlice(str string, values []string) bool {
 	}
 	return false
 }
+
+// ContainedInSlice returns true if fullstring is contained within any element of the candidates slice. False otherwise
+func ContainedInSlice(fullString string, candidates []string) bool {


name is not that clear. if you saw two functions InSlice and ContainedInSlice, could you tell what the difference was?

I'd call this something like SubstringInSlice

also please use the same var names as InSlice (str, values) for consistency, unless different names would add clarity.

lyoshenka · 2018-07-19T14:46:20Z

ytsync/manager.go

+}
+
+const (
+	StatusPending = "pending" // waiting for permission to sync


perhaps these constants should be shared with internal-apis? then they're not defined in two places.

good idea! should i put them in a package of their own?

Actually, the way they reside in the code will make them available through the ytsync package once this is merged in master. So nothing to change here I believe

lyoshenka · 2018-07-19T14:48:27Z

ytsync/manager.go

+	if !response.Data.IsNull() && response.Data.String == "ok" {
+		return nil
+	}
+	return errors.Err("invalid API response")


maybe include the erroneous response in the error?

lyoshenka · 2018-07-19T15:04:09Z

ytsync/ytsync.go

 	}
+	defer func() {


this is pretty confusing. id rather put the rest of FullCycle into another function, call that function, then check the error message and run this deferred stuff on it

lyoshenka · 2018-07-19T15:05:46Z

ytsync/ytsync.go

+			if err != nil {
+				msg := fmt.Sprintf("Failed setting failed state for channel %s.", s.LbryChannelName)
+				err = errors.Prefix(msg, err)
+				e = errors.Prefix(err.Error(), e)


its kinda redundant to have two Prefix-ings. you can just do e = errors.Prefix(msg + ":" + err.Error(), e)

lyoshenka · 2018-07-19T15:07:30Z

ytsync/ytsync.go

+				e = err
+			}
+		}
+	}()

 	defaultWalletDir := os.Getenv("HOME") + "/.lbryum/wallets/default_wallet"


since you now have a SyncManager that gets configured, this stuff should come from the SyncManager. it should not be set here. as before, calling os.Getenv() here is weird. I know my code did this before, but that should be changed now.

lyoshenka · 2018-07-19T15:08:29Z

ytsync/ytsync.go

@@ -175,13 +209,17 @@ WaitForDaemonStart:

 	return nil
 }
+func logShutdownError(shutdownErr error) {


👍

the other functions you added to util/slack.go should go here as well

lyoshenka · 2018-07-19T15:08:52Z

ytsync/ytsync.go

-				if s.StopOnError {
+				logMsg := fmt.Sprintf("error processing video: " + err.Error())
+				log.Errorln(logMsg)
+				fatalErrors := []string{


fatalErrorSubstrings

add video marking support

nikooo777 added this to the May 9 (web) milestone Apr 30, 2018

lbry-bot assigned nikooo777 Apr 30, 2018

nikooo777 force-pushed the automation-support branch from e4a795c to f44442e Compare May 16, 2018 23:42

nikooo777 requested a review from lyoshenka May 17, 2018 22:46

lbry-bot assigned lyoshenka and unassigned nikooo777 May 17, 2018

lyoshenka reviewed May 23, 2018

View reviewed changes

lbry-bot assigned nikooo777 and unassigned lyoshenka and nikooo777 May 23, 2018

kauffj modified the milestones: May 9 (web), May 30 (web) May 23, 2018

lbry-bot assigned nikooo777 and unassigned nikooo777 May 25, 2018

nikooo777 force-pushed the automation-support branch from bf3adb1 to 03fe5d2 Compare May 26, 2018 00:50

lbry-bot assigned nikooo777 and unassigned nikooo777 May 29, 2018

nikooo777 force-pushed the automation-support branch from 03fe5d2 to df554b7 Compare June 4, 2018 14:36

nikooo777 force-pushed the automation-support branch from 44c83d4 to b64b8d5 Compare June 17, 2018 23:51

lyoshenka force-pushed the master branch 3 times, most recently from 865d7b8 to 2134a81 Compare June 27, 2018 20:27

nikooo777 added 5 commits July 12, 2018 08:26

resolve conflict changes in rebase

fb748cb

fix slack logging

d596240

add failure cases change interrupt behavior for fatal errors bump up timeout to 20 minutes change max file size to 2GB remove unnecessary utxo wait

remove launch parameter

3cf504d

improve failure handling adjust slack logging increase file size in string increase publish timeout to 40 minutes

resolve rebase conflicts

45bb0f2

update dependencies

4a4008d

nikooo777 force-pushed the automation-support branch from b64b8d5 to 4a4008d Compare July 12, 2018 13:04

remove common.go

dfa3bb1

merge sync and selfsync export sync manager out of cmd package clean up ytsync.go address #19 and #20

nikooo777 mentioned this pull request Jul 17, 2018

extract slack logging to own package #22

Open

lbry-bot assigned nikooo777 and unassigned nikooo777 Jul 17, 2018

nikooo777 force-pushed the automation-support branch from 906c909 to a273b65 Compare July 17, 2018 20:23

nikooo777 requested a review from lyoshenka July 18, 2018 12:42

lbry-bot assigned lyoshenka and unassigned nikooo777 Jul 18, 2018

remove invalid tests

f800e77

move functions out of utils address review comments revert lockfile changes

nikooo777 force-pushed the automation-support branch from a273b65 to f800e77 Compare July 18, 2018 12:47

lyoshenka reviewed Jul 19, 2018

View reviewed changes

lbry-bot assigned nikooo777 and unassigned lyoshenka and nikooo777 Jul 19, 2018

nikooo777 added 2 commits July 20, 2018 19:56

rename selfsync to ytsync

915862c

add video marking support

address review comments

7322d40

lyoshenka merged commit 7322d40 into master Jul 25, 2018

lyoshenka deleted the automation-support branch July 25, 2018 14:38

nikooo777 mentioned this pull request Aug 2, 2018

Address old review comments #31

Closed

11 tasks

This was referenced Nov 1, 2018

Address old review comments lbryio/ytsync#4

Closed

Address old review comments lbryio/ytsync#5

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Automation support #10

Automation support #10

nikooo777 commented Apr 30, 2018

tzarebczan commented May 2, 2018

nikooo777 commented May 2, 2018

nikooo777 commented May 17, 2018

lyoshenka left a comment

lyoshenka May 23, 2018

nikooo777 May 25, 2018

lyoshenka May 23, 2018

lyoshenka May 23, 2018

lyoshenka May 23, 2018

nikooo777 Jul 17, 2018

lyoshenka May 23, 2018

lyoshenka May 23, 2018

lyoshenka May 23, 2018

lyoshenka May 23, 2018

nikooo777 May 25, 2018

lyoshenka May 23, 2018

lyoshenka May 23, 2018

lyoshenka Jul 19, 2018

nikooo777 Jul 19, 2018

lyoshenka Jul 19, 2018

lyoshenka Jul 19, 2018

lyoshenka Jul 19, 2018

nikooo777 Jul 19, 2018

nikooo777 Jul 23, 2018

lyoshenka Jul 19, 2018

lyoshenka Jul 19, 2018

lyoshenka Jul 19, 2018

lyoshenka Jul 19, 2018

lyoshenka Jul 19, 2018

lyoshenka Jul 19, 2018

Automation support #10

Automation support #10

Conversation

nikooo777 commented Apr 30, 2018

tzarebczan commented May 2, 2018

nikooo777 commented May 2, 2018

nikooo777 commented May 17, 2018

lyoshenka 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

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

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

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