-
Notifications
You must be signed in to change notification settings - Fork 2.1k
validate_third_party_test: do not use main package #15605
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
Conversation
|
Hello! I am a robot. Tests will require approval from a repository maintainer to run. Googlers: For automatic test runs see go/terraform-auto-test-runs. @slevenick, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look. You can help make sure that review is quick by doing a self-review and by running impacted tests locally. |
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR hasn't generated any diffs, but I'll let you know if a future commit does. |
melinath
left a comment
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.
changing the package name is definitely fine, and the implementation changes are... probably fine? a couple nits/questions
`main` makes it sound like this is a test for the main.go entrypoint and this confuses Bazel automated BUILD file generation code. Instead, create a `test/` subpackage and run the test from there. Notes: * Code claimed to validate `third_party/` templates but in practice was validating `third_party/terraform` only: clean up variables and comments to make this clearer. * Because the test is moved one directory deeper, there is now a need for one more `Dir()` call to bubble one directory higher (relatively) to find `./third_party/` compared to the test file.
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR hasn't generated any diffs, but I'll let you know if a future commit does. |
melinath
left a comment
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.
LGTM - I'm not sure we want this to live in a test directory long-term but I don't know what the best spot would be. Putting it next to main.go is definitely wrong, and third_party isn't a go package. So... this seems fine for now. We can always move it later if we want to.
mainmakes it sound like this is a test for the main.go entrypoint andthis confuses Bazel automated BUILD file generation code.
Instead, create a
test/subpackage and run the test from there.Notes:
third_party/templates but in practice wasvalidating
third_party/terraformonly: clean up variables and commentsto make this clearer.
for one more
Dir()call to bubble one directory higher (relatively)to find
./third_party/compared to the test file.Tested: with local tests, and I also locally edited a template (into not a template) to verify test still fails as expected.
Release Note Template for Downstream PRs (will be copied)
See
Write release notes
for guidance.