Skip to content

Conversation

BanaSeba
Copy link
Collaborator

@BanaSeba BanaSeba commented Sep 4, 2025

  • Update Railway ENSAdmin version on deploy
  • Add ENSAdmin on Render

@BanaSeba BanaSeba requested a review from a team as a code owner September 4, 2025 20:24
Copy link

changeset-bot bot commented Sep 4, 2025

⚠️ No Changeset found

Latest commit: 3f21b37

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

vercel bot commented Sep 4, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
admin.ensnode.io Ready Ready Preview Comment Sep 6, 2025 2:08pm
ensnode.io Ready Ready Preview Comment Sep 6, 2025 2:08pm
ensrainbow.io Ready Ready Preview Comment Sep 6, 2025 2:08pm

Copy link
Member

@lightwalker-eth lightwalker-eth left a comment

Choose a reason for hiding this comment

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

@BanaSeba Excited to see this as it's a key priority! Reviewed and shared feedback 👍

render_region = local.render_region
render_environment_id = render_project.ensnode.environments["default"].id
instance_type = "starter"

Copy link
Member

Choose a reason for hiding this comment

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

What about environment variables that ENSAdmin uses?

See: https://github.com/namehash/ensnode/blob/main/apps/ensadmin/.env.local.example

Shouldn't all 3 of those be referenced here in some way?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added ANTHROPIC_API_KEY . NEXT_PUBLIC_DEFAULT_ENSNODE_URLS is not working for Docker images. We will need to fix it first. Not sure if the name will still be the same

Copy link
Member

Choose a reason for hiding this comment

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

Where can I find a GitHub issue to track the issue with supporting NEXT_PUBLIC_DEFAULT_ENSNODE_URLS in Docker images?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've created the issue for that

@@ -0,0 +1,16 @@
locals {
full_ensadmin_hostname = "ensadmin.${var.subdomain_prefix}.${var.base_domain_name}"
Copy link
Member

Choose a reason for hiding this comment

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

There's a number of places where I see us write terminology such as "full ... hostname", etc..

My understanding is that the correct technical term for this is "fully qualified domain name" or "fqdn" as an abbreviation.

Suggest to refine naming across our terraform scripts to make use of "fqdn" where relevant. You're welcome to action this idea in this PR or to create a new GitHub issue for it and follow up later in a separate PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed to fqdn

tag = var.ensnode_version
}
}

Copy link
Member

Choose a reason for hiding this comment

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

What about environment variables that ENSAdmin uses?

See: https://github.com/namehash/ensnode/blob/main/apps/ensadmin/.env.local.example

Shouldn't all 3 of those be referenced here in some way?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added ANTHROPIC_API_KEY . NEXT_PUBLIC_DEFAULT_ENSNODE_URLS is not working for Docker images. We will need to fix it first. Not sure if the name will still be the same

}
}

custom_domains = [
Copy link
Member

Choose a reason for hiding this comment

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

Could you please add a comment explaining what this custom_domains idea is about?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added Domains assigned by user

Copy link
Member

Choose a reason for hiding this comment

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

Please see related feedback on the same idea in another file

@@ -0,0 +1,3 @@
output "ensadmin_url" {
Copy link
Member

Choose a reason for hiding this comment

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

Is this related to the ENSADMIN_PUBLIC_URL environment variable?

Assuming so, we should give it the same name. Ex: ensadmin_public_url. The "public" in this context is a hint how the URL needs to be usable for the public internet and not a private network.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed to ensadmin_public_url

type = string
}

# DNS configuration
Copy link
Member

Choose a reason for hiding this comment

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

Please add more documentation for this DNS configuration. It's not clear to me what it's really used for / what purpose or consequence it has.

This comment applies to both the base_domain_name and subdomain_prefix fields.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added more comments

Copy link
Member

Choose a reason for hiding this comment

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

Thanks. That helps. Please see related feedback on how we name these fields in another file.

Copy link
Member

@lightwalker-eth lightwalker-eth left a comment

Choose a reason for hiding this comment

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

@BanaSeba Thanks for updates. Reviewed and shared feedback

},
ENSINDEXER_URL = {
value = "http://ensindexer-${var.instance_name}:10000"
}
}
)
# Domains assigned by user
Copy link
Member

Choose a reason for hiding this comment

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

I'm completely lost when I read this comment. Please put more effort into these comments.

Who is the user?

What does it mean to assign a domain in this context?

Assign a domain to what?

For what purpose does this logic exist?

Why are these called "custom_domains"?

@@ -19,6 +19,9 @@ variable "ensdb_disk_size_gb" {
type = number
default = 250
}
variable "anthropic_api_key" {
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way to define which variables are optional?

For example, "anthropic_api_key" should be optional.

@@ -19,6 +19,9 @@ variable "ensdb_disk_size_gb" {
type = number
default = 250
}
variable "anthropic_api_key" {
Copy link
Member

Choose a reason for hiding this comment

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

This variable is only relevant for ENSAdmin. It would be nice to document that here. It shouldn't be listed under "General Variables"

@@ -96,7 +98,7 @@ resource "render_web_service" "ensindexer_api" {
local.common_variables,
{
ENSNODE_PUBLIC_URL = {
value = "https://${local.full_ensindexer_api_hostname}"
value = "https://${local.ensindexer_api_fqdn}"
},
ENSINDEXER_URL = {
value = "http://ensindexer-${var.instance_name}:10000"
Copy link
Member

Choose a reason for hiding this comment

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

I'm finding this instance_name var unclear. Instance name for what exactly? The instance name of the ENSIndexer instance within 1 ENSNode deployment (that combines 1 api instance and 1 indexer instance)? Or is this the instance name of the ENSIndexer instance within an overall environment (such as the overall yellow environment)?

What is the possible range of values for instance_name?

@@ -107,7 +109,7 @@ resource "render_web_service" "ensindexer_api" {
}
)
custom_domains = [
Copy link
Member

Choose a reason for hiding this comment

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

Please see my related comments about custom_domains in another file.

We need to write all our code, including all our terraform code, so it has strong documentation and can be clearly understood by people who are new to the code and new to terraform.

I keep finding too many issues where these goals are failing.

}
}

custom_domains = [
Copy link
Member

Choose a reason for hiding this comment

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

Please see related feedback on the same idea in another file

type = string
}

variable "anthropic_api_key" {
Copy link
Member

Choose a reason for hiding this comment

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

The anthropic api key shouldn't be listed under "# Render configuration"

type = string
}

# DNS configuration
Copy link
Member

Choose a reason for hiding this comment

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

Thanks. That helps. Please see related feedback on how we name these fields in another file.

@@ -13,6 +13,7 @@ locals {
"INDEX_ADDITIONAL_RESOLVER_RECORDS" = { value = var.index_additional_resolver_records },
"HEAL_REVERSE_ADDRESSES" = { value = var.heal_reverse_addresses },
"REPLACE_UNNORMALIZED" = { value = var.replace_unnormalized },
"ENSADMIN_URL" = { value = var.ensadmin_public_url }
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, why is ensadmin_public_url a completely customizable URL? Shouldn't we be able to build it up programmatically based on other variables? Or is there a special reason we have to do it this way?

@@ -45,6 +45,10 @@ variable "ensrainbow_url" {
type = string
}

variable "ensadmin_public_url" {
Copy link
Member

Choose a reason for hiding this comment

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

Please see related comment asking why we can't deterministically build this URL out of other variables?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants