Conversation
changelog: Internal, Reporting,sensitive tag to all columns
samathad2023
force-pushed
the
lg-14425-pii-column-tag
branch
from
e51a701 to
f680c75
Compare
.gitlab-ci.yml
Outdated
| script: | ||
| - *bundle_install | ||
| - bundle exec rake db:create db:migrate --trace | ||
| - bundle exec rake db:check_for_sensitive_columns |
There was a problem hiding this comment.
what if we added this code as a spec instead? spec/db/schema_spec.rb and had it do the same check? easier to see output in tests or re-run
spec/db/schema_spec.rb
Outdated
| around do |ex| | ||
| ex.run | ||
| rescue SystemExit | ||
| end |
There was a problem hiding this comment.
I was missing this piece to actually swallow the systemExit.
| if missing_columns.any? | ||
| puts 'Columns with sensitivity comments found:' | ||
| missing_columns.each { |column| puts column } | ||
| exit(-1) |
There was a problem hiding this comment.
swapped to exit instead of abort w/message because I'd have to spy the Kernal to avoid sysout
| it 'checks for columns with sensitivity comments' do | ||
| expect { task.execute }.to output(/All columns have sensitivity comments./).to_stdout | ||
| end |
There was a problem hiding this comment.
should we update the description or failure message to have example so that when this fails, people have clear instructions to follow to add comments in their migrations and fix the errors?
something like
in your migration file, add `comment: "sensitive=false"` to all columns added
There was a problem hiding this comment.
Yes, I suppose I either shouldn't suppress puts or leave a comment in the tests to run the task locally.
There was a problem hiding this comment.
maybe there's a way to capture the STDOUT and then put that as the test expectation? This would just print the string it got instead
| it 'checks for columns with sensitivity comments' do | |
| expect { task.execute }.to output(/All columns have sensitivity comments./).to_stdout | |
| end | |
| it 'checks for columns with sensitivity comments' do | |
| stdout = StringIO.new('') | |
| stub_const(STDOUT, stdout) | |
| task.execute | |
| expect(stdout.string).to match(/All columns have sensitivity comments./) | |
| end |
There was a problem hiding this comment.
TIL if you expect output after a rake task executes with an exit(1)... it will always pass.
There was a problem hiding this comment.
TBH I'm not 100% sure that a rake task is the best approach for this, especially if we want to test exit codes and things, that gets very challenging. Just a spec that checks the columsn would be fine
There was a problem hiding this comment.
Yeah, I don't think we want to test the exit codes. The rake task is nice to run as a one-off. I think it's in a good spot now.
|
|
||
| it 'displays the missing column directions' do | ||
| expect { task.execute }.to output( | ||
| /In your migration, add 'comment: sensitive=false'\(or true for sensitive data\)/, |
There was a problem hiding this comment.
I'm ensuring a test would fail with the correct output statement.
3 participants
🎫 Ticket
Link to the relevant ticket:
LG-14425
🛠 Summary of changes
Migrations to add sensitive tag to all columns and failure on CI on none sensitive tag detention
📜 Testing Plan
Provide a checklist of steps to confirm the changes.
👀 Screenshots
Helpful test error.