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

Fix: Support app auth and PAT via two provider instances #1538

Merged
merged 3 commits into from
Feb 17, 2023

Conversation

MajorBreakfast
Copy link
Contributor

@MajorBreakfast MajorBreakfast commented Feb 9, 2023

Resolves #1537


Behavior

Before the change?

The following code errors if both GITHUB_APP_... and GITHUB_TOKEN env vars are set:

provider "github" { # Uses GITHUB_APP_... env vars
  app_auth {}
}

The error message looks like this:

│ Error: no decodeable PEM data found
│ 
│   with provider["registry.terraform.io/hashicorp/github"],
│   on main.tf line 1, in provider "github":
│    1: provider "github" {

After the change?

  • app_auth {} block set => Provider uses the GITHUB_APP_... env vars (currently the case*), ignores GITHUB_TOKEN env var (new)
  • app_auth {} block not set => Provider uses the GITHUB_TOKEN env var (currently the case*), ignores GITHUB_APP_... env vars (currently the case*)

Other information


Additional info

Pull request checklist

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Added the appropriate label for the given change

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

  • Yes (Please add the Type: Breaking change label)
  • No

If Yes, what's the impact:

  • N/A

Pull request type

Please add the corresponding label for change this PR introduces:

  • Bugfix: Type: Bug
  • Feature/model/API additions: Type: Feature
  • Updates to docs or samples: Type: Documentation
  • Dependencies/code cleanup: Type: Maintenance

@MajorBreakfast MajorBreakfast changed the title Support app auth and PAT via two provider instances Fix: Support app auth and PAT via two provider instances Feb 9, 2023
@kfcampbell
Copy link
Member

This PR does not take into account your proposed alias keyword in #1537. Do you view this as a transitional step? Are you going to be updating this PR?

@nickfloyd nickfloyd added the Type: Feature New feature or request label Feb 10, 2023
@kfcampbell
Copy link
Member

Alright, I've spent a little more time looking into this. I can confirm the changes are as described:

  • On main
    • With app_auth set, without token set, operation succeeds
    • With app_auth set, with token set, operation fails with the ConflictsWith error
  • On branch
    • With app_auth set, without token set, operation succeeds
    • With app_auth set, with token set, operation succeeds

The PR that introduced app auth, #753, mentions that the app_auth block "conflicts deliberately" with the token, but doesn't specify why.

If this change unblocks others, I'm happy to ship it.

@MajorBreakfast
Copy link
Contributor Author

@kfcampbell

Thanks!

This PR does not take into account your proposed alias keyword in #1537

alias is just Terraform's built-in way to distinguish between multiple instances of the same provider. It exists out-of-the-box on any provider.

@MajorBreakfast
Copy link
Contributor Author

@kfcampbell any progress? Looking forward to next gh provider release with a fix :)

@MajorBreakfast
Copy link
Contributor Author

@kfcampbell Is there an update? Looking forward to seeing this in a new version

Copy link
Member

@kfcampbell kfcampbell left a comment

Choose a reason for hiding this comment

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

I'll get this merged now and released soon.

@kfcampbell kfcampbell merged commit bd36a1f into integrations:main Feb 17, 2023
reedloden pushed a commit to reedloden/terraform-provider-github that referenced this pull request Jun 14, 2023
Co-authored-by: Nick Floyd <139819+nickfloyd@users.noreply.github.com>
Co-authored-by: Keegan Campbell <me@kfcampbell.com>
avidspartan1 pushed a commit to avidspartan1/terraform-provider-github that referenced this pull request Feb 5, 2024
Co-authored-by: Nick Floyd <139819+nickfloyd@users.noreply.github.com>
Co-authored-by: Keegan Campbell <me@kfcampbell.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEAT]: Support app auth and PAT via two provider instances
3 participants