Herald.go #47

Merged
jeffreypicard merged 5 commits from herald.go into master 2022-08-09 13:43:02 +02:00
jeffreypicard commented 2022-08-04 21:10:36 +02:00 (Migrated from github.com)
  • Changes to the codebase for the second repo rename.
  • Updates to the ci/cd scripts and docker files for static builds
* Changes to the codebase for the second repo rename. * Updates to the ci/cd scripts and docker files for static builds
lyoshenka (Migrated from github.com) reviewed 2022-08-04 21:10:36 +02:00
moodyjon (Migrated from github.com) reviewed 2022-08-04 22:31:03 +02:00
moodyjon (Migrated from github.com) left a comment

The issue with "go build" producing herald.go is a problem because it has a name like a source file. Could we rename the repository a second time to "go-herald" or "herald-go"?

The issue with "go build" producing herald.go is a problem because it has a name like a source file. Could we rename the repository a second time to "go-herald" or "herald-go"?
@ -4,7 +4,7 @@ EXPOSE 50051
RUN mkdir /app
WORKDIR /app
COPY . /app
moodyjon (Migrated from github.com) commented 2022-08-04 22:07:07 +02:00

Should this be changed also? It looks like it would be building herald.go at line #7.

See also https://github.com/lbryio/herald.go/blob/master/docker/Dockerfile.action where there is some commented out reference to "/hub".

Should this be changed also? It looks like it would be building herald.go at line #7. See also https://github.com/lbryio/herald.go/blob/master/docker/Dockerfile.action where there is some commented out reference to "/hub".
@ -1,4 +1,4 @@
module github.com/lbryio/herald
module github.com/lbryio/herald.go
moodyjon (Migrated from github.com) commented 2022-08-04 22:27:14 +02:00

I'm not sure about this, but I thing think is what controls the default output file (-o) for "go build".

Now that I think about it, having the binary be named "herald.go" is a potential source of confusion. Name looks like it could be a .go source file which you might open.

I'm not sure about this, but I thing think is what controls the default output file (-o) for "go build". Now that I think about it, having the binary be named "herald.go" is a potential source of confusion. Name looks like it could be a .go source file which you might open.
moodyjon (Migrated from github.com) commented 2022-08-04 22:03:08 +02:00

Is this changing because build.sh is downloading a different version of protoc-gen-go-grpc?

Stuff in lbryio/types has not changed much recently.

Is this changing because build.sh is downloading a different version of protoc-gen-go-grpc? Stuff in lbryio/types has not changed much recently.
@ -31,3 +31,2 @@
os.system(cmd)
with open("./hub", "rb") as f:
release.upload_asset("binary", "hub", f)
with open("./herald", "rb") as f:
moodyjon (Migrated from github.com) commented 2022-08-04 22:13:18 +02:00

Another /hub reference. Should be /herald or /herald.go.

Another /hub reference. Should be /herald or /herald.go.
jeffreypicard (Migrated from github.com) reviewed 2022-08-05 14:41:44 +02:00
@ -1,4 +1,4 @@
module github.com/lbryio/herald
module github.com/lbryio/herald.go
jeffreypicard (Migrated from github.com) commented 2022-08-05 14:41:43 +02:00

Yeah, this is definitely a problem, you're right there's confusion after the herald.go is generate.

Yeah, this is definitely a problem, you're right there's confusion after the herald.go is generate.
jeffreypicard commented 2022-08-05 14:42:33 +02:00 (Migrated from github.com)

The issue with "go build" producing herald.go is a problem because it has a name like a source file. Could we rename the repository a second time to "go-herald" or "herald-go"?

We've already renamed it twice haha, I think this time was being X.go is the standard naming convention, so we'll just have to figure out how to fix the binary at this point I think.

> The issue with "go build" producing herald.go is a problem because it has a name like a source file. Could we rename the repository a second time to "go-herald" or "herald-go"? We've already renamed it twice haha, I think this time was being X.go is the standard naming convention, so we'll just have to figure out how to fix the binary at this point I think.
jeffreypicard (Migrated from github.com) reviewed 2022-08-05 14:43:33 +02:00
jeffreypicard (Migrated from github.com) commented 2022-08-05 14:43:33 +02:00

Yes, I believe so. Perhaps also due to the move to go 1.18. Let's talk about which direction we want to sync these up in in the call.

Yes, I believe so. Perhaps also due to the move to go 1.18. Let's talk about which direction we want to sync these up in in the call.
jeffreypicard (Migrated from github.com) reviewed 2022-08-05 16:32:29 +02:00
@ -4,7 +4,7 @@ EXPOSE 50051
RUN mkdir /app
WORKDIR /app
COPY . /app
jeffreypicard (Migrated from github.com) commented 2022-08-05 16:32:28 +02:00

Yup, good catch.

Yup, good catch.
moodyjon commented 2022-08-08 16:02:10 +02:00 (Migrated from github.com)

Looks good. Only open question is why the .pb.go files changed.

Looks good. Only open question is why the .pb.go files changed.
jeffreypicard commented 2022-08-09 13:42:16 +02:00 (Migrated from github.com)

Looks good. Only open question is why the .pb.go files changed.

So I think it's a moot point now actually, he package name in the .proto had to change anyways actually so it had to change the generated files. I'm going to merge this, pull in your change from the other one, and then cut a new version.

> Looks good. Only open question is why the .pb.go files changed. So I think it's a moot point now actually, he package name in the .proto had to change anyways actually so it had to change the generated files. I'm going to merge this, pull in your change from the other one, and then cut a new version.
Sign in to join this conversation.
No reviewers
No milestone
No project
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/herald.go#47
No description provided.