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

IPS & ood-portal-generator: Change whitelist to allowlist #583

Merged
merged 4 commits into from
Sep 15, 2021

Conversation

matthu017
Copy link
Contributor

Deals with ips and ood_portal_generator

@matthu017 matthu017 linked an issue Jul 13, 2020 that may be closed by this pull request
@matthu017 matthu017 changed the title Change whitelist to allowlist to be more descriptive IPS & ood-portal-generator: Change whitelist to allowlist Jul 13, 2020
@ericfranz ericfranz added this to the OOD2.0 milestone Aug 4, 2020
@matthu017 matthu017 added ready and removed blocked labels Aug 8, 2020
@ericfranz ericfranz modified the milestones: OOD2.0, OOD2.1 Oct 20, 2020
johrstrom
johrstrom previously approved these changes Dec 30, 2020
Copy link
Contributor

@johrstrom johrstrom left a comment

Choose a reason for hiding this comment

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

I fixed the merge conflict and rebased to master. I think we're good with this, there's a test case testing the current config and the previous config against the same file, so it's in good shape.

not only does this add tests, but starts a testing infrastructure
around testing input and output fixture files.
@johrstrom
Copy link
Contributor

Can I get a second pair of eyes on this @treydock?

I added test cases that utilize fixture files rather than stubbing context. I kinda like testing those boundaries, rather than stubbing classes. In fact, I'd kinda like to refactor this a little towards that.

@johrstrom
Copy link
Contributor

Do you have any thoughts on the testing strategy I'd like to move towards? Input fixture files are small enough and won't need to change alot - but output fixture files would need changing across potentially all of them. That's kind of the situation we're in now, but I feel like this library's purpose is to write a file given an input file - so let's test those boundaries.

@treydock
Copy link
Contributor

I think it makes to stub files and test whole files since that's how the application is operating, it works off of files and writes out whole files. The huge benefit to validating entire files is to ensure we know 100% how certain changes affect the entire templates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use allowlist and blocklist
5 participants