LBRYCommunity/lbry.go
1
0
Fork 0
Code Issues 5 Pull requests Projects Releases 46 Packages Wiki Activity

Automation support #10

Merged
nikooo777 merged 31 commits from automation-support into master 2018-07-25 16:37:30 +02:00
Conversation 4 Commits 31 Files changed 10 +644 -140
nikooo777 commented 2018-04-30 21:28:04 +02:00 (Migrated from github.com)
Copy link

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

This addresses https://github.com/lbryio/internal-issues/issues/165
tzarebczan commented 2018-05-02 18:41:51 +02:00 (Migrated from github.com)
Copy link

@nikooo777 Is this ready for review?

@nikooo777 Is this ready for review?
nikooo777 commented 2018-05-02 23:30:10 +02:00 (Migrated from github.com)
Copy link

negative Tom :) it's work in progress

negative Tom :) it's work in progress
nikooo777 commented 2018-05-18 00:49:25 +02:00 (Migrated from github.com)
Copy link

@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 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 (Migrated from github.com) reviewed 2018-05-23 16:09:00 +02:00
lyoshenka (Migrated from github.com) left a comment
Copy link

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.

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.
cmd/common.go
lyoshenka (Migrated from github.com) commented 2018-05-23 16:07:33 +02:00
Copy link

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

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
cmd/selfsync.go
lyoshenka (Migrated from github.com) commented 2018-05-23 15:42:06 +02:00
Copy link

what is this?

what is this?
lyoshenka (Migrated from github.com) commented 2018-05-23 15:43:23 +02:00
Copy link

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

don't hardcode this url. the API domain and endpoints should be configured somewhere
lyoshenka (Migrated from github.com) commented 2018-05-23 16:06:45 +02:00
Copy link

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.

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.
cmd/selfsync_test.go
lyoshenka (Migrated from github.com) commented 2018-05-23 16:04:02 +02:00
Copy link

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?

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?
util/disk_space.go
lyoshenka (Migrated from github.com) commented 2018-05-23 15:46:06 +02:00
Copy link

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

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
util/slack.go
lyoshenka (Migrated from github.com) commented 2018-05-23 15:47:23 +02:00
Copy link

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.

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 (Migrated from github.com) commented 2018-05-23 15:52:18 +02:00
Copy link

this should be SendErrorToSlack. and really, it should be it's own package called slack so you can simply slack.Error("stuff") instead of util.SendErrorToSlack("stuff")

this should be SendErrorToSlack. and really, it should be it's own package called `slack` so you can simply `slack.Error("stuff")` instead of `util.SendErrorToSlack("stuff")`
lyoshenka (Migrated from github.com) commented 2018-05-23 16:02:51 +02:00
Copy link

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

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
util/slack_test.go
lyoshenka (Migrated from github.com) commented 2018-05-23 15:47:57 +02:00
Copy link

this doesn't test anything

this doesn't test anything
util/slice.go
lyoshenka (Migrated from github.com) commented 2018-05-23 15:56:28 +02:00
Copy link

this needs a better name

this needs a better name
ytsync/setup.go
@ -39,1 +42,4 @@
log.Debugf("Source channel has %d videos", numOnSource)
if numOnSource == 0 {
return nil
}
lyoshenka (Migrated from github.com) commented 2018-05-23 15:48:32 +02:00
Copy link

why?

why?
ytsync/setup.go
@ -140,6 +156,7 @@ func (s *Sync) ensureEnoughUTXOs() error {
}
func (s *Sync) waitUntilUTXOsConfirmed() error {
origin := time.Now()
lyoshenka (Migrated from github.com) commented 2018-05-23 15:49:19 +02:00
Copy link

restarting the daemon should be its own function

restarting the daemon should be its own function
ytsync/setup.go
lyoshenka (Migrated from github.com) commented 2018-05-23 15:49:29 +02:00
Copy link

does this still happen with lbryumx?

does this still happen with lbryumx?
ytsync/sources/youtubeVideo.go
@ -197,3 +206,3 @@
func (v YoutubeVideo) Sync(daemon *jsonrpc.Client, claimAddress string, amount float64, channelName string) error {
func (v YoutubeVideo) Sync(daemon *jsonrpc.Client, claimAddress string, amount float64, channelName string) (*SyncSummary, error) {
//download and thumbnail can be done in parallel
err := v.download()
lyoshenka (Migrated from github.com) commented 2018-05-23 15:50:27 +02:00
Copy link

err is not a delete error here, its the publish error

err is not a delete error here, its the publish error
ytsync/ytsync.go
lyoshenka (Migrated from github.com) commented 2018-05-23 15:57:26 +02:00
Copy link

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

use `errors.Prefix` so you don't lose the original trace
ytsync/ytsync.go
@ -254,1 +330,3 @@
log.Println(err.Error())
s.grp.Stop()
SendErrorToSlack("Failed to setup the wallet for a refill: %v", err)
break
lyoshenka (Migrated from github.com) commented 2018-05-23 15:54:49 +02:00
Copy link

what i meant was, why not just download the thumbnail from youtube to s3. why do we need some remote JS function running to do this?

also, if you're gonna answer questions in comments, delete the question and simply explain what the code is doing.

what i meant was, why not just download the thumbnail from youtube to s3. why do we need some remote JS function running to do this? also, if you're gonna answer questions in comments, delete the question and simply explain what the code is doing.
nikooo777 (Migrated from github.com) reviewed 2018-05-25 02:15:33 +02:00
cmd/selfsync_test.go
nikooo777 (Migrated from github.com) commented 2018-05-25 02:15:33 +02:00
Copy link

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.

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.
nikooo777 (Migrated from github.com) reviewed 2018-05-25 02:18:00 +02:00
ytsync/setup.go
@ -39,1 +42,4 @@
log.Debugf("Source channel has %d videos", numOnSource)
if numOnSource == 0 {
return nil
}
nikooo777 (Migrated from github.com) commented 2018-05-25 02:18:00 +02:00
Copy link

The main reason is that while we publish for 0.01 LBC, the fees are up to 0.1LBC.
In a future approach we'll have to change this to just calculate the fees and add 0.01LBC on top of it

The main reason is that while we publish for 0.01 LBC, the fees are up to 0.1LBC. In a future approach we'll have to change this to just calculate the fees and add 0.01LBC on top of it
nikooo777 (Migrated from github.com) reviewed 2018-05-25 02:28:18 +02:00
cmd/selfsync.go
nikooo777 (Migrated from github.com) commented 2018-05-25 02:28:18 +02:00
Copy link

the other solution we discussed worked too :)

the other solution we discussed worked too :)
lyoshenka (Migrated from github.com) reviewed 2018-05-25 16:59:31 +02:00
ytsync/setup.go
@ -39,1 +42,4 @@
log.Debugf("Source channel has %d videos", numOnSource)
if numOnSource == 0 {
return nil
}
lyoshenka (Migrated from github.com) commented 2018-05-25 16:59:31 +02:00
Copy link

oh, that makes sense. when i wrote this, we did not have fees based on claim name length

oh, that makes sense. when i wrote this, we did not have fees based on claim name length
nikooo777 (Migrated from github.com) reviewed 2018-05-29 19:49:42 +02:00
ytsync/setup.go
nikooo777 (Migrated from github.com) commented 2018-05-29 19:49:42 +02:00
Copy link

https://github.com/lbryio/lbry/issues/1192#issuecomment-392816078

https://github.com/lbryio/lbry/issues/1192#issuecomment-392816078
kauffj commented 2018-07-11 16:57:20 +02:00 (Migrated from github.com)
Copy link

@lyoshenka if @nikooo777 is using this in production does it really make sense to continue to block merging this?

@lyoshenka if @nikooo777 is using this in production does it really make sense to continue to block merging this?
nikooo777 (Migrated from github.com) reviewed 2018-07-17 20:30:16 +02:00
util/slack.go
nikooo777 (Migrated from github.com) commented 2018-07-17 20:30:16 +02:00
Copy link

https://github.com/lbryio/lbry.go/issues/22

https://github.com/lbryio/lbry.go/issues/22
lyoshenka (Migrated from github.com) reviewed 2018-07-19 17:09:22 +02:00
cmd/selfsync.go
lyoshenka (Migrated from github.com) commented 2018-07-19 16:37:46 +02:00
Copy link

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

why the switch from `ytsync` to `selfsync`. This package is called `lbry.go`, so its very general. `selfsync` is too generic a name.
util/slack.go
@ -37,8 +46,11 @@ func SendToSlackChannel(channel, username, message string) error {
}
lyoshenka (Migrated from github.com) commented 2018-07-19 16:41:38 +02:00
Copy link

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.

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.
util/slice.go
lyoshenka (Migrated from github.com) commented 2018-07-19 16:45:23 +02:00
Copy link

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.

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.
ytsync/manager.go
lyoshenka (Migrated from github.com) commented 2018-07-19 16:46:20 +02:00
Copy link

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

perhaps these constants should be shared with internal-apis? then they're not defined in two places.
lyoshenka (Migrated from github.com) commented 2018-07-19 16:48:27 +02:00
Copy link

maybe include the erroneous response in the error?

maybe include the erroneous response in the error?
lyoshenka (Migrated from github.com) commented 2018-07-19 16:50:00 +02:00
Copy link

os.Getenv() should not be used here. that should happen in cmd. syncmanager should be initialized with the correct keys. that way it can be used in tests, in other packages, etc

os.Getenv() should not be used here. that should happen in `cmd`. syncmanager should be initialized with the correct keys. that way it can be used in tests, in other packages, etc
lyoshenka (Migrated from github.com) commented 2018-07-19 16:50:42 +02:00
Copy link

blobfiles dir should be configurable

blobfiles dir should be configurable
ytsync/setup.go
@ -37,6 +40,9 @@ func (s *Sync) walletSetup() error {
}
lyoshenka (Migrated from github.com) commented 2018-07-19 16:52:18 +02:00
Copy link

fyi this can happen if the channel owner also publishes other videos into the channel

fyi this can happen if the channel owner also publishes other videos into the channel
ytsync/sources/youtubeVideo.go
lyoshenka (Migrated from github.com) commented 2018-07-19 16:55:25 +02:00
Copy link

checking the message is not the right way to detect if the dir exists. google it or ask me if you're not sure

checking the message is not the right way to detect if the dir exists. google it or ask me if you're not sure
lyoshenka (Migrated from github.com) commented 2018-07-19 16:56:53 +02:00
Copy link

v.dir+"/"+v.id should be a function, since its used in multiple places. maybe videoDir()?

`v.dir+"/"+v.id` should be a function, since its used in multiple places. maybe `videoDir()`?
ytsync/sources/youtubeVideo.go
@ -195,3 +204,3 @@
}
func (v YoutubeVideo) Sync(daemon *jsonrpc.Client, claimAddress string, amount float64, channelName string) error {
func (v YoutubeVideo) Sync(daemon *jsonrpc.Client, claimAddress string, amount float64, channelName string) (*SyncSummary, error) {
lyoshenka (Migrated from github.com) commented 2018-07-19 16:58:42 +02:00
Copy link

why are we ignoring the error? do errors happen here? i'd at least log it. same above.

why are we ignoring the error? do errors happen here? i'd at least log it. same above.
ytsync/ytsync.go
@ -116,3 +170,4 @@
logShutdownError(processDeathError)
} else {
walletErr := os.Rename(defaultWalletDir, walletBackupDir)
if walletErr != nil {
lyoshenka (Migrated from github.com) commented 2018-07-19 17:04:09 +02:00
Copy link

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

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
ytsync/ytsync.go
lyoshenka (Migrated from github.com) commented 2018-07-19 17:05:46 +02:00
Copy link

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

its kinda redundant to have two Prefix-ings. you can just do `e = errors.Prefix(msg + ":" + err.Error(), e)`
ytsync/ytsync.go
lyoshenka (Migrated from github.com) commented 2018-07-19 17:07:30 +02:00
Copy link

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.

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.
ytsync/ytsync.go
lyoshenka (Migrated from github.com) commented 2018-07-19 17:08:29 +02:00
Copy link

👍

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

:+1: the other functions you added to util/slack.go should go here as well
ytsync/ytsync.go
lyoshenka (Migrated from github.com) commented 2018-07-19 17:08:52 +02:00
Copy link

fatalErrorSubstrings

`fatalErrorSubstrings`
nikooo777 (Migrated from github.com) reviewed 2018-07-19 17:17:39 +02:00
cmd/selfsync.go
nikooo777 (Migrated from github.com) commented 2018-07-19 17:17:38 +02:00
Copy link

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.

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.
nikooo777 (Migrated from github.com) reviewed 2018-07-19 17:21:27 +02:00
ytsync/manager.go
nikooo777 (Migrated from github.com) commented 2018-07-19 17:21:27 +02:00
Copy link

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

good idea! should i put them in a package of their own?
nikooo777 (Migrated from github.com) reviewed 2018-07-19 17:22:11 +02:00
ytsync/manager.go
nikooo777 (Migrated from github.com) commented 2018-07-19 17:22:11 +02:00
Copy link

Ok! I'll move them there

Ok! I'll move them there
nikooo777 (Migrated from github.com) reviewed 2018-07-19 17:22:27 +02:00
ytsync/manager.go
nikooo777 (Migrated from github.com) commented 2018-07-19 17:22:27 +02:00
Copy link

Will export it to an env var

Will export it to an env var
nikooo777 (Migrated from github.com) reviewed 2018-07-19 17:23:50 +02:00
ytsync/setup.go
@ -37,6 +40,9 @@ func (s *Sync) walletSetup() error {
}
nikooo777 (Migrated from github.com) commented 2018-07-19 17:23:50 +02:00
Copy link

yes if we'll allow publishing to channels while we also keep them in sync then this will be the case. otherwise this happens when channel owners delete videos from their youtube channel

yes if we'll allow publishing to channels while we also keep them in sync then this will be the case. otherwise this happens when channel owners delete videos from their youtube channel
nikooo777 (Migrated from github.com) reviewed 2018-07-19 17:25:44 +02:00
ytsync/sources/youtubeVideo.go
@ -195,3 +204,3 @@
}
func (v YoutubeVideo) Sync(daemon *jsonrpc.Client, claimAddress string, amount float64, channelName string) error {
func (v YoutubeVideo) Sync(daemon *jsonrpc.Client, claimAddress string, amount float64, channelName string) (*SyncSummary, error) {
nikooo777 (Migrated from github.com) commented 2018-07-19 17:25:44 +02:00
Copy link

the error is logged within the function. I had the same thoughts the other day and couldn't understand my reasoning until when I checked the function itself. Perhaps I should drop logging within the function and log it here? Also we ignore the error because deleting is not something that will compromise publishing and we can take care of that "offline"

the error is logged within the function. I had the same thoughts the other day and couldn't understand my reasoning until when I checked the function itself. Perhaps I should drop logging within the function and log it here? Also we ignore the error because deleting is not something that will compromise publishing and we can take care of that "offline"
nikooo777 (Migrated from github.com) reviewed 2018-07-24 01:14:14 +02:00
ytsync/manager.go
nikooo777 (Migrated from github.com) commented 2018-07-24 01:14:14 +02:00
Copy link

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

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
nikooo777 (Migrated from github.com) reviewed 2018-07-24 01:32:02 +02:00
ytsync/sources/youtubeVideo.go
nikooo777 (Migrated from github.com) commented 2018-07-24 01:32:02 +02:00
Copy link

Actually, this is not meant to check if a directory exists, instead it's meant to report any error other than "file exists". This is due to the fact that each video has its own subdirectory to avoid name conflicts and if for some reasons the video was previously downloaded (ie on a retry attempt) the directory doesn't have to be re-created.

If however you think I should check for the directory before calling mkdir then I will adapt the code.

Actually, this is not meant to check if a directory exists, instead it's meant to report any error other than "file exists". This is due to the fact that each video has its own subdirectory to avoid name conflicts and if for some reasons the video was previously downloaded (ie on a retry attempt) the directory doesn't have to be re-created. If however you think I should check for the directory before calling mkdir then I will adapt the code.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
area: devops

   
area: discovery

   
area: docs

   
area: livestream

   
area: proposal

Work is part of a proposal

  
consider soon

Discuss this issue at the next planning meeting, then remove this label

  
dependencies

Pull requests that update a dependency file

  
Epic

   
good first issue

   
hacktoberfest

Welcome to Hacktoberfest

  
help wanted

   
icebox

Long-term storage

  
level: 1

No knowledge of the existing code required

  
level: 2

Some knowledge of the existing code is recommended

  
level: 3

Significant knowledge of the existing code is recommended

  
level: 4

Intimate knowledge of the existing code is recommended

  
needs: exploration

Solution unclear, needs research

  
needs: grooming

Issue needs to be groomed before work can start

  
needs: priority

Priority level needs to be defined

  
needs: repro

Issue needs step-by-step instructions on how to reproduce in latest code

  
needs: tech design

Needs technical design signoff before implementation can begin

  
on hold

Temporarily paused

  
priority: blocker

Issue is blocking release, do ASAP

  
priority: high

Work needs to be moved into sprint ASAP

  
priority: low

Work should be done but can stay in the backlog for now

  
priority: medium

Work needs to be done within 2-3 sprints

  
resilience

   
Tom's Wishlist

   
type: bug

Existing functionality is wrong or broken

  
type: discussion

A conversation. No specific changes requested

  
type: improvement

Existing (or partially existing) functionality needs to be changed

  
type: new feature

New functionality that does not exist yet

  
type: refactor

Minimal user-visible changes, but significant internal work

  
type: task

Either work that's not related to the code, or a small chore that does not fit into other categories

  
type: testing

Solution needs additional user testing

  
unplanned

Work that was not planned into the spirnt, took priority over planned work

No labels
area: devops
area: discovery
area: docs
area: livestream
area: proposal
consider soon
dependencies
Epic
good first issue
hacktoberfest
help wanted
icebox
level: 1
level: 2
level: 3
level: 4
needs: exploration
needs: grooming
needs: priority
needs: repro
needs: tech design
on hold
priority: blocker
priority: high
priority: low
priority: medium
resilience
Tom's Wishlist
type: bug
type: discussion
type: improvement
type: new feature
type: refactor
type: task
type: testing
unplanned
Milestone
Clear milestone
No items
No milestone
July 18th (API)
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: LBRYCommunity/lbry.go#10
No description provided.
Delete branch "automation-support"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?