Skip to content

website: Enable CPEx on staging and development branches #4056

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

g0at1
Copy link
Contributor

@g0at1 g0at1 commented Apr 12, 2025

Context

Closes #4051. CPEx should be enabled on branches that are not production to facilitate better testing.

Implementation

I added new variable isProduction and use it to enable CPEx on non production branches.

Other Information

Copy link

vercel bot commented Apr 12, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
nusmods-export ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 12, 2025 9:12pm

Copy link

vercel bot commented Apr 12, 2025

@g0at1 is attempting to deploy a commit to the modsbot's projects Team on Vercel.

A member of the Team first needs to authorize it.

Copy link

codecov bot commented Apr 12, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 54.82%. Comparing base (988c6fd) to head (e0411db).
Report is 99 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4056      +/-   ##
==========================================
+ Coverage   54.52%   54.82%   +0.30%     
==========================================
  Files         274      276       +2     
  Lines        6076     6351     +275     
  Branches     1455     1556     +101     
==========================================
+ Hits         3313     3482     +169     
- Misses       2763     2869     +106     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@g0at1 g0at1 changed the title Enable CPEx on staging and development branches website: Enable CPEx on staging and development branches Apr 12, 2025
Copy link
Member

@jloh02 jloh02 left a comment

Choose a reason for hiding this comment

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

Thanks for this update. However there's 2 problems that don't quite seem right to me

  1. Doing this in featureFlags.ts doesn't seem right as this no longer becomes a feature flag
  2. This implicitly breaks for production and the change away from a manual feature flag implies that it should be a fixed value

To solve this issue I would propose either of 2 alternatives:

  1. Move this logic out featureFlags and convert the feature flag to something like "enableCPExForProd"; or
  2. Create time based system for CPEx (which is difficult but preferred)

If we are keen on option 2 in this case (ie anyone deems the effort to reward ratio is worth to implement this), perhaps, opening a larger issue in place of #4050 and #4051 is better.

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.

Enable CPEx on staging and development branches
2 participants