-
Notifications
You must be signed in to change notification settings - Fork 20
FFS-4713 Add an environment variable to disable NSC in a given environment, forcing the Education flow to self-attestation only. #2047
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
base: main
Are you sure you want to change the base?
Changes from 7 commits
1ca43b1
130ec07
5c9e0ff
386305a
c9ce219
63fccc2
20378fa
8008e84
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -48,3 +48,4 @@ MISSION_CONTROL_USER=user | |
| MISSION_CONTROL_PASSWORD=password | ||
|
|
||
| STRUCTURED_LOGGING_ENABLED=false | ||
| NSC_DISABLED=false | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| module Nsc | ||
| def self.disabled? | ||
| ActiveModel::Type::Boolean.new.cast(ENV["NSC_DISABLED"]) || false | ||
|
krista-skylight marked this conversation as resolved.
Outdated
|
||
| end | ||
|
|
||
| def self.enabled? | ||
| !disabled? | ||
| end | ||
| end | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. test-classifier: AI triage of failing tests AI Test Classifier — no action required
Used the precomputed AI_REVIEW_DIFF_RANGE (origin/main HEAD), which correctly captured PR #2047's NSC_DISABLED feature-flag change (23 files); no base re-resolution was needed. Reproduced CI's rspec job (Ruby 3.4.10, Postgres 16 container, npm ci + rake assets:precompile, RAILS_ENV=test) and ran React 👍 if this is right (nothing needed triage) / 👎 if a real failure was missed, and on a 👎 please reply with a one-line reason. Advisory, non-blocking.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. test-classifier: AI triage of failing tests AI Test Classifier — no action required
Bootstrapped per the repo's own CI (postgres:16 service container, rake db:schema:load, npm install + rake assets:precompile) and ran the CI test command React 👍 if this is right (nothing needed triage) / 👎 if a real failure was missed, and on a 👎 please reply with a one-line reason. Advisory, non-blocking.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. test-classifier: AI triage of failing tests AI Test Classifier — no action required
Used the dispatcher-provided range origin/main..HEAD as-is; it correctly captured the PR #2047 change (NSC_DISABLED kill switch for education flows, 26 files), so no base re-resolution was needed. Reproduced CI's rspec.yml path: started Postgres, installed gems, npm install, rake assets:precompile, db:schema:load, then parallel_test spec/ -n 4 --exclude-pattern '^spec/e2e/' (same e2e exclusion CI uses; e2e runs in a separate workflow requiring browsers/external aggregators). Result: 2584 examples, 0 failures across all 4 processes; the 9 changed spec files alone give 339 examples, 0 failures. rubocop (664 files) and erb_lint (124 files) are also clean. Two bootstrap caveats: the Gemfile pins Ruby 3.4.9 but only 3.4.10 is available in the runner toolchain, so tests ran under 3.4.10 via a throwaway sidecar Gemfile.ci (deleted afterward, no tracked file modified); and an initial run performed before assets:precompile produced ~120 spurious Sprockets AssetNotFound errors in every view-rendering spec, which was an artifact of my own incomplete bootstrap rather than the change under test and which disappeared entirely once assets were compiled as CI does, so those are not classified. Nothing failed, so classifications is empty. React 👍 if this is right (nothing needed triage) / 👎 if a real failure was missed, and on a 👎 please reply with a one-line reason. Advisory, non-blocking. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| require "rails_helper" | ||
|
|
||
| RSpec.describe Nsc do | ||
| describe ".disabled?" do | ||
| it "returns false when NSC_DISABLED is unset" do | ||
| stub_environment_variable("NSC_DISABLED", nil) do | ||
| expect(described_class.disabled?).to be false | ||
| end | ||
| end | ||
|
|
||
| it "returns false when NSC_DISABLED is 'false'" do | ||
| stub_environment_variable("NSC_DISABLED", "false") do | ||
| expect(described_class.disabled?).to be false | ||
| end | ||
| end | ||
|
|
||
| it "returns false when NSC_DISABLED is '0'" do | ||
| stub_environment_variable("NSC_DISABLED", "0") do | ||
| expect(described_class.disabled?).to be false | ||
| end | ||
| end | ||
|
|
||
| it "returns true when NSC_DISABLED is 'true'" do | ||
| stub_environment_variable("NSC_DISABLED", "true") do | ||
| expect(described_class.disabled?).to be true | ||
| end | ||
| end | ||
|
|
||
| it "returns true when NSC_DISABLED is '1'" do | ||
| stub_environment_variable("NSC_DISABLED", "1") do | ||
| expect(described_class.disabled?).to be true | ||
| end | ||
| end | ||
|
|
||
| it "returns true when NSC_DISABLED is 't'" do | ||
| stub_environment_variable("NSC_DISABLED", "t") do | ||
| expect(described_class.disabled?).to be true | ||
| end | ||
| end | ||
| end | ||
|
|
||
| describe ".enabled?" do | ||
| it "returns true when NSC_DISABLED is unset" do | ||
| stub_environment_variable("NSC_DISABLED", nil) do | ||
| expect(described_class.enabled?).to be true | ||
| end | ||
| end | ||
|
|
||
| it "returns false when NSC_DISABLED is 'true'" do | ||
| stub_environment_variable("NSC_DISABLED", "true") do | ||
| expect(described_class.enabled?).to be false | ||
| end | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good for a local default and testing, but we're going to need to set this in AWS Parameter Store and load the setting in each environment that way. Take a look at infra/app/app-config/env-config/environment-variables.tf to see how we do that with other variables like SITE_ALERT_ENABLED. Take a gander at the PR that added that for a guide on what's needed.
(And, in addition, we will need to go through and create this variable in Parameter Store in each environment, and set it to false.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I got it done. How do I go about getting credentials to create the variable in parameter store?