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

Enable white-listing features & tests and self-hosted agents #350

Merged
merged 25 commits into from
Oct 7, 2019

Conversation

ThePhen
Copy link
Contributor

@ThePhen ThePhen commented Oct 1, 2019

All Submissions:


  • [YES] Have you followed the guidelines in our Contributing document?
  • [YES] Have you added an explanation of what your changes do and why you'd like us to include them?
  • [YES] I have updated the documentation accordingly.
  • [YES] I have added tests to cover my changes.
  • [YES] All new and existing tests passed.
  • [YES] My code follows the code style of this project.
  • [YES] I ran lint checks locally prior to submission.
  • [YES] Have you checked to ensure there aren't other open Pull Requests for the same update/change?

What is the current behavior?


Resolves Issue Number: #339 and #341

What is the new behavior?


  • a value for resource_ip_whitelist must be provided by users (e.g. by setting TF_VAR_resource_ip_whitelist to ["x.x.x.x/32"], or via *.tfvars`, etc). Build pipelines fail with a clear error, if a value is not provided.
  • white listing of IPs is enabled, and unit tests cover this.
  • this PR fixes the handling (by git) of PNG files by making sure they are treated as binaries (this manifested as a problem on some Windows dev machines)
  • in conversation with the team, a decision was made to clean-up what deserved to be tested in a unit test, and what deserved to be tested during integration testing, specifically the capability to manipulate the IP White Lists were kept for unit tests, but no explicit IP addresses are expected for integration tests (i.e. the verifyIPWhitelistForACR tests at .../acr.go and .../keyvault.gowere removed, in favor of the new IP White List testing inaz-isolated-service-single-region/tests/unit/unit_test.go`)
  • some work from Remove ACR Webhooks; Do not manage container settings via Terraform #324, related to retiring some ACR Web Hook-related code needed to be cherry-picked into this PR.

Does this introduce a breaking change?


  • [NO]

@ThePhen ThePhen requested a review from erikschlegel as a code owner October 1, 2019 20:23
@ThePhen ThePhen changed the title Feature/242 Enable white-listing features & tests and self-hosted agents Oct 1, 2019
@ThePhen ThePhen added the wip label Oct 3, 2019
@ThePhen ThePhen removed the wip label Oct 7, 2019
@ThePhen
Copy link
Contributor Author

ThePhen commented Oct 7, 2019

@erikschlegel has pointed out that some solid integration tests, which check that the IP While Lists are as expected at Integration Test-time, are needed, and that's a great point.

I'd suggest that such test get written as a follow-on tasks, given that the team chose the unit vs. integration test strategy for this PR, and this PR is blocking some other work.

I do believe I know what needs to be written to enact the integration test that Erik's calling out.

Copy link
Contributor

@ianphil ianphil left a comment

Choose a reason for hiding this comment

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

Create a backlog item, not V1, for creating a whitelist integration test. 🚢 it

@ThePhen ThePhen merged commit a5ff6b7 into master Oct 7, 2019
@nmiodice nmiodice deleted the feature/242 branch April 14, 2020 19:07
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.

4 participants