Make data_github_repository work with non-existing repositories #1031
Conversation
jcudit
added
test/TestAccGithubRepositoryDataSource
and removed
test/TestAccGithubRepositoryDataSource
labels
There was a problem hiding this comment.
Looks close!
As for testing, I think it makes sense to delete the TestAccGithubRepositoryDataSource/raises_expected_errors_when_querying_for_a_repository test given the nature of this change. Feel free to 🔥 it on your next pass. All other tests in that file were passing for me.
|
Thank you so much for the feedback, @jcudit! I'll have another stab at it next week. 👍 Although, I think I tweaked that specific test so as to verify that the data resource works with non-existent repos. Perhaps it should be renamed? Or do you not see a big need for it in the test suite? |
There was a problem hiding this comment.
This change might be worthy of a new major version; it feels backwards-incompatible although it's more permissive.
@tobiassjosten I agree, the tests are somewhat confusing and not always reliable. I've found I get the most confidence from writing Terraform configuration and using the instructions in our Contributing.md to validate that the changes made are working.
|
I couldn't quite figure out what to test with this change and with both @jcudit and @kfcampbell recommending deleting the one affected test, that's what I did. I'll admit I still haven't run through the testing steps in CONTRIBUTING.md so that's what I'll do next. The code should be done, though. |
|
Acceptance tests are working fine and I've rebased and updated the dependencies to be in line with the evolving main branch. Are we good to merge? :) |
…grations#1031) * Make data_github_repository work with non-existing repositories * Improve 404 logging * Remove obsolete test * Update dependency
…grations#1031) * Make data_github_repository work with non-existing repositories * Improve 404 logging * Remove obsolete test * Update dependency
Fixes #933 – github_repository needs to not fail if repository doesn't exist.
This is my first stab at working with a Terraform provider, so bear with me. :)
I began by updating the tests and even with this new condition and no changes to the code, they still worked. So I'm not sure how useful they actually are. As for the implementation itself, I referenced c240bcc for the logic but since I don't trust the tests too much I'm not entirely sure how to best verify the change. Please advice!