Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changes generated hostname for compute-go bundles #8

Merged
merged 2 commits into from
Apr 5, 2024

Conversation

gunsch
Copy link
Collaborator

@gunsch gunsch commented Apr 5, 2024

💸 TL;DR

When doing local development, gRPC calls to [hostname].localhost:1337 can be processed by compute-go. However, this only works if the hostname is one DNS "part", so bundles named e.g. untitled-1712273914361.local break this.

Changing this to untitled-1712273914361 makes this path possible again, and makes local testing possible without gRPC-web.

🧪 Testing Steps / Validation

Validated this in local development. This request doesn't require any metadata to succeed.

image

✅ Checks

  • CI tests (if present) are passing
  • Adheres to code style for repo
  • Contributor License Agreement (CLA) completed if not a Reddit employee

When doing local development, gRPC calls to `[hostname].localhost:1337` can be processed by compute-go. However, this only works if the `hostname` is one DNS "part", so bundles named e.g. `untitled-1712273914361.local` break this.

Changing this to `untitled-1712273914361-local` makes this path possible again.
@gunsch gunsch requested a review from niedzielski April 5, 2024 22:11
Copy link
Member

@niedzielski niedzielski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great! Would you mind documenting the gRPC workflow in briefly in the internal development workflows doc?

@@ -1,5 +1,5 @@
// to-do: compute-go doesn't seem to replace old bundles. Generate a unique
// hostname for each new build.
export function newHostname(name: string, version: number): string {
return `${name.toLocaleLowerCase().replace(/[^a-z0-9]/, '') || 'untitled'}-${version}.local`
return `${name.toLocaleLowerCase().replace(/[^a-z0-9]/, '') || 'untitled'}-${version}-local`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was mistakenly under the impression that this hostname had to end in .local for compute. Can you just delete the entire -local? I don't think it's adding any value.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good! deleted, re-confirmed it still works as expected

drops -local entirely
Copy link
Member

@niedzielski niedzielski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Merge it!

@gunsch gunsch merged commit 3e48614 into main Apr 5, 2024
1 check passed
@gunsch gunsch deleted the gunsch-change-uploaded-hostname branch April 5, 2024 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants