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

Kayrus optional parameters #255

Merged
merged 9 commits into from
Jul 22, 2019
Merged

Kayrus optional parameters #255

merged 9 commits into from
Jul 22, 2019

Conversation

megan07
Copy link
Contributor

@megan07 megan07 commented Jul 10, 2019

Addressing #230, #205, and #190 where GITHUB_TOKEN should not be required for all data sources.
Before fix:

        Error: GET https://api.github.com/users/hashibot: 401 Bad credentials []

          on /var/folders/nj/n5hr70hd2dx45qb__6wl10nc0000gp/T/tf-test353694711/main.tf line 6:
          (source code not available)


FAIL

After fix:

== RUN   TestProvider_anonymous
--- PASS: TestProvider_anonymous (1.04s)
PASS

@ghost ghost added size/M Type: Documentation Improvements or additions to documentation labels Jul 10, 2019
@megan07 megan07 requested a review from a team July 10, 2019 17:18
github/provider.go Outdated Show resolved Hide resolved
github/provider.go Outdated Show resolved Hide resolved
@@ -90,6 +102,59 @@ func TestProvider_individual(t *testing.T) {
Config: individualProviderConfig + testAccGithubMembershipConfig(username),
ExpectError: regexp.MustCompile("This resource requires GitHub organization to be set on the provider."),
},
{
Config: conflictingProviderConfig + testAccCheckGithubUserDataSourceConfig(username),
ExpectError: regexp.MustCompile("If `individual` is true, `organization` cannot be set."),
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this test pass? conflictingProviderConfig, from what I can see, only sets individual to true, it doesn't set an organization.

`

neitherProviderConfig := `provider "github" {
organization = ""
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the more common user scenario, and the one we should probably be testing for, is that they didn't specify organization at all, rather than setting it to an empty string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@paddycarver I agree, however, the rest of the tests need GITHUB_ORGANIZATION defined, so, when running the suite it will already be there, this will override that. Is there a better way to do this?

@ghost ghost added size/L and removed size/M labels Jul 16, 2019
@kayrus
Copy link

kayrus commented Aug 8, 2019

@megan07 when are you going to make the release with this feature?

@radeksimko radeksimko deleted the kayrus-optional-parameters branch November 25, 2019 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/L Type: Documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants