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

[Cloudflare] Remove netlify #2669

Merged
merged 7 commits into from
Jan 20, 2025
Merged

[Cloudflare] Remove netlify #2669

merged 7 commits into from
Jan 20, 2025

Conversation

adamgall
Copy link
Member

@adamgall adamgall commented Jan 14, 2025

This PR finally removes Netlify from the codebase.

My plan is to merge this (assuming approvals) on Sunday Night January 19th and shut down the Netlify sites at that time.

Copy link

netlify bot commented Jan 14, 2025

Deploy Preview for decent-interface-dev failed. Why did it fail? →

Name Link
🔨 Latest commit 91a63ec
🔍 Latest deploy log https://app.netlify.com/sites/decent-interface-dev/deploys/67886bb081fcbe0008ae3bb8

Copy link

cloudflare-workers-and-pages bot commented Jan 14, 2025

Deploying decent-interface with  Cloudflare Pages  Cloudflare Pages

Latest commit: 8763c18
Status: ✅  Deploy successful!
Preview URL: https://3e806634.decent-interface.pages.dev
Branch Preview URL: https://cf-remove-netlify.decent-interface.pages.dev

View logs

@adamgall adamgall force-pushed the cf/remove-netlify branch 3 times, most recently from 6d789a4 to 47e53cd Compare January 14, 2025 09:04
@adamgall adamgall force-pushed the cf/remove-netlify branch 2 times, most recently from 92dfb9c to 3f82e36 Compare January 15, 2025 17:23
@adamgall adamgall changed the base branch from cf/new-demo-mode to cloudflare January 15, 2025 17:23
Base automatically changed from cloudflare to develop January 15, 2025 21:19
@adamgall adamgall marked this pull request as ready for review January 16, 2025 03:08
Copy link
Contributor

@mudrila mudrila left a comment

Choose a reason for hiding this comment

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

Yeah lezzgo 🤘🏼
Couple minor suggestions for cleaning up env file - but not mandatory within a scope of this PR for sure

.env Outdated
# IPFS pinning
VITE_APP_INFURA_IPFS_API_KEY=""
# IPFS pinning
VITE_APP_INFURA_IPFS_API_SECRET=""

# App name displayed as Title
VITE_APP_NAME="Decent"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this one? Don't feel like it's reasonable to have it in env variable

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 say we need it because it's a global level configuration string that is used in multiple places throughout the app.

If not stored as an Env Var, what would we do instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just a constant somewhere
My point is that it is not environment-dependant - app name is always Decent for every environment

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah so I hear you, but I would argue to keep it here because:

  • we use it multiple times in the index.html file, and in there we do have access to environment variables, but we don't have access to any javascript code
  • also, by setting VITE_APP_NAME="Decent" in the .env file, the build system will pick up "Decent" if it's not overridden in a real environment variable

@adamgall adamgall self-assigned this Jan 17, 2025
@adamgall adamgall merged commit b0b3401 into develop Jan 20, 2025
4 checks passed
@adamgall adamgall deleted the cf/remove-netlify branch January 20, 2025 14:06
@adamgall adamgall mentioned this pull request Jan 21, 2025
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.

3 participants