Skip to content

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

Merged
kfcampbell merged 3 commits intointegrations:mainfrom
MajorBreakfast:pat-and-app-auth
Feb 17, 2023
Merged

Fix: Support app auth and PAT via two provider instances#1538
kfcampbell merged 3 commits intointegrations:mainfrom
MajorBreakfast:pat-and-app-auth

Conversation

@MajorBreakfast
Copy link
Copy Markdown
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
Copy Markdown
Contributor

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
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
Contributor Author

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

@MajorBreakfast
Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Contributor

@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