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

Storacha-branded validation flow #417

Merged
merged 21 commits into from
Sep 19, 2024
Merged

Storacha-branded validation flow #417

merged 21 commits into from
Sep 19, 2024

Conversation

Peeja
Copy link
Member

@Peeja Peeja commented Aug 19, 2024

Before Merge:


With NEXT_PUBLIC_W3UP_SERVICE_URL=https://petra.up.storacha.network in my local console:

CleanShot 2024-08-19 at 17 33 15

Then…

CleanShot 2024-08-19 at 17 34 12

And success!

CleanShot 2024-08-19 at 17 33 55

Or, failure.

CleanShot 2024-08-19 at 17 34 19

Note that that error message is a dummy message in the test harness, not something real. Also, I see to have bumped something that made the Stripe section always appear. Previously, staging/prod was set not to show it by not providing the right env vars. I'm not sure why that changed, and whether we want it to.

Closes storacha/project-tracking#119

@Peeja
Copy link
Member Author

Peeja commented Aug 19, 2024

Lint errors, please hold…

Copy link

seed-deploy bot commented Aug 20, 2024

View stack outputs

@Peeja
Copy link
Member Author

Peeja commented Aug 20, 2024

@alanshaw Not sure what's failing here. It should be unrelated. Are these tests a bit flaky, or did I bump something?

Copy link
Member

@alanshaw alanshaw left a comment

Choose a reason for hiding this comment

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

This looks pretty good to me, I've left a suggestion for simplifying the hosted zone config...

The styling needs a bit of work but is good enough for now!

.env.tpl Outdated
@@ -2,7 +2,8 @@

# uncomment to try out deploying the w3up api under a custom domain.
# the value should match a hosted zone configured in route53 that your aws account has access to.
# HOSTED_ZONE=up.dag.haus
# HOSTED_ZONE=up.web3.storage
# HOSTED_ZONE_STORACHA=up.storacha.network
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion, make HOSTED_ZONE a CSV and map over the values. This would work for 1 domain or 2 or more.

The BRANDING env var could just be the domain.

Then we don't have implementation specific env vars and we don't add a new one!

Copy link
Member Author

Choose a reason for hiding this comment

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

Do we actually want to support multiple domains? I envisioned this as something very temporary. Note that we have forking code paths for the emails and Preact pages. I don't think we actually want something like that indefinitely, just long enough to cover the launch of Storacha. What's in this PR seemed to me like the easiest thing to tear out later. Making HOSTED_ZONE plural is a bunch of logic if we don't actually want to keep it for long.

Copy link
Member

Choose a reason for hiding this comment

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

Do we actually want to support multiple domains? I envisioned this as something very temporary.

I don't know for sure. In the past we've wanted to theme nft.storage emails, and we literally want this now. I don't see a lot of harm in retaining the logic. I find that temporary becomes permanent more often than I expect.

Note that we have forking code paths for the emails and Preact pages. I don't think we actually want something like that indefinitely, just long enough to cover the launch of Storacha. What's in this PR seemed to me like the easiest thing to tear out later.

I think I also suggested the forking could instead just be a keyed map of config (or at least I meant to). It would be quicker to not tear this out later and just reconfigure...

Making HOSTED_ZONE plural is a bunch of logic if we don't actually want to keep it for long.

I guess yes, but I feel like pulling this out vs a plural HOSTED_ZONE would be approximately the same effort.

const customDomainStoracha = getCustomDomain(
stack.stage,
process.env.HOSTED_ZONE_STORACHA
)
Copy link
Member

Choose a reason for hiding this comment

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

const customDomains = process.env.HOSTED_ZONE.split(',').map(domain => getCustomDomain(stack.stage, domain))

action_url: opts.url,
environment_name: this.environment,
},
}
Copy link
Member

Choose a reason for hiding this comment

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

Config object keyed on the hostname?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd prefer to do it this way at two, so it defaults more easily. My strategy here was to always default to the new branding if an unknown value is set. We could throw and blow up the function, but I'm not sure that's worth it here.

If we ever extend this to three, I'm all for it, and it's a pretty easy, local change at that point.

@Peeja
Copy link
Member Author

Peeja commented Aug 22, 2024

@alanshaw Rereview? WDYT?

@Peeja Peeja requested a review from alanshaw August 22, 2024 20:20
@Peeja
Copy link
Member Author

Peeja commented Aug 29, 2024

@hannahhoward Also, you're right that there are references to HOSTED_ZONE which I missed. Now that I'm looking at them, I could use some direction. I don't fully understand the Filecoin and Roundabout stacks yet; what are they doing with that URL and what should they do when there are multiple "brands" to choose from?

Notably, there's one HOSTED_ZONE which is fine: the validate-email function takes a HOSTED_ZONE env var for its own hosted zone. It's instantiated for each of the zones configured.

As for reading from ApiEndpoints…apparently I didn't push that commit. 🤦🏻‍♀️ It's up now. 😅

@hannahhoward
Copy link
Member

@Peeja for the filecoin and roundabout stacks, these are entirely API services with no direct interaction with the client. Honestly, they should probably all be on the storacha.network domain, but for now we can just avoid any change and use HOSTED_ZONES.split(",")[0]

currently, seed.run fails 90%+ of the time because of the Filecoin test.

until we have a more reliable way to test the filecoin flow, the PR disables it.

the inserted comment suggests a few solutions -- either spinning up
custom infra for w3filecoin pipeline on each pr to get more reliable
results, or mocking out the pipeline
Copy link

seed-deploy bot commented Sep 2, 2024

Stack outputs updated

@seed-deploy seed-deploy bot temporarily deployed to pr417 September 6, 2024 14:45 Inactive
@hannahhoward hannahhoward merged commit 3749515 into main Sep 19, 2024
3 checks passed
@hannahhoward hannahhoward deleted the storacha-emails branch September 19, 2024 19:20
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.

email round trip and pricing page rebrand
3 participants