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.

Sign up for GitHub

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

Jump to bottom

Add new datasource to get GitHub App token #1671

Merged
merged 5 commits into from
Jun 15, 2023
Merged

Add new datasource to get GitHub App token #1671

merged 5 commits into from
Jun 15, 2023

Conversation

phillebaba
Copy link
Contributor

Resolves #1656

Behavior

Before the change?

It was not possible to generate a GitHub App JWT to use outside of the GitHub provider.

After the change?

The new datasource enables Terraform users to pass a GitHub App JWT to other provider resources so that they are able to authenticate with the GitHub API.

Other information

N/A

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

kfcampbell
kfcampbell reviewed May 5, 2023
View reviewed changes
examples/app_token/main.tf Show resolved Hide resolved
github/data_source_github_app_token.go Outdated Show resolved Hide resolved
github/data_source_github_app_token.go
}
}

func dataSourceGithubAppTokenRead(d *schema.ResourceData, meta interface{}) error {
Copy link
Member

Choose a reason for hiding this comment

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

Can you create appropriate testing for this please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To test this I need a proper GitHub App. Would you say its a good idea to use the same setup that already exists to test the providers app auth?

https://github.com/integrations/terraform-provider-github/blob/main/github/apps_test.go

Copy link
Member

Choose a reason for hiding this comment

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

I'm 👍 on reusing the same testing configuration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found some issues with how to test this. Either we do this with real credentials which is not present in the current test scope. Or we solve this like the provider tests and mock the client response. The issue is that using httptest would require changing the base url, which is set in the provider scope.

Do you have any other good idea for testing this datasource?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kfcampbell any thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

I like the idea of mocking the client response.

The issue is that using httptest would require changing the base url, which is set in the provider scope.

I'm not quite sure changing the base URL is an issue in tests. We have several tests that change the base URL, like this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kfcampbell thanks for pointing me in the right direction. I have now implemented a test that validates the token creation with the mocked API endpoint. I removed any JWT validation because it would be impossible to actually have a static JWT. Hopefully this test is good enough.

website/docs/d/app_token.html.markdown Show resolved Hide resolved
@phillebaba phillebaba force-pushed the feature/data-app-token branch from c0f0107 to 0bcb971 Compare May 11, 2023 16:04
@phillebaba phillebaba force-pushed the feature/data-app-token branch from 0bcb971 to f53f86a Compare June 8, 2023 09:29
@phillebaba phillebaba requested a review from kfcampbell June 8, 2023 19:31
kfcampbell
kfcampbell approved these changes Jun 15, 2023
View reviewed changes
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.

@phillebaba thanks for working with me here!

@kfcampbell kfcampbell merged commit 42ebbe0 into integrations:main Jun 15, 2023
doonga referenced this pull request in doonga/greyrock-ops Jun 17, 2023
@greyrock-repo-duster
feat(terraform-provider): update terraform github to 5.28.0 (#790)
f811826 
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [github](https://registry.terraform.io/providers/integrations/github)
([source](https://github.com/integrations/terraform-provider-github))
| required_provider | minor | `5.27.0` -> `5.28.0` |

---

### Release Notes

<details>
<summary>integrations/terraform-provider-github</summary>

###
[`v5.28.0`](https://github.com/integrations/terraform-provider-github/releases/tag/v5.28.0)

[Compare
Source](https://github.com/integrations/terraform-provider-github/compare/v5.27.0...v5.28.0)

#### What's Changed

- Add note documenting weird permissions required by
[@&#8203;kfcampbell](https://github.com/kfcampbell) in
[https://github.com/integrations/terraform-provider-github/pull/1727](https://github.com/integrations/terraform-provider-github/pull/1727)
- Add new datasource to get GitHub App token by
[@&#8203;phillebaba](https://github.com/phillebaba) in
[https://github.com/integrations/terraform-provider-github/pull/1671](https://github.com/integrations/terraform-provider-github/pull/1671)
- fix: GitHub Repository Update might fail when Pages enabled by
[@&#8203;0x46616c6b](https://github.com/0x46616c6b) in
[https://github.com/integrations/terraform-provider-github/pull/1716](https://github.com/integrations/terraform-provider-github/pull/1716)

#### New Contributors

- [@&#8203;phillebaba](https://github.com/phillebaba) made their first
contribution in
[https://github.com/integrations/terraform-provider-github/pull/1671](https://github.com/integrations/terraform-provider-github/pull/1671)

**Full Changelog**:
integrations/terraform-provider-github@v5.27.0...v5.27.1

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Renovate
Bot](https://github.com/renovatebot/renovate).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS4xMjMuMCIsInVwZGF0ZWRJblZlciI6IjM1LjEyMy4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->

Co-authored-by: greyrock-bot <1583719+greyrock-bot[bot]@users.noreply.github.com>
avidspartan1 pushed a commit to avidspartan1/terraform-provider-github that referenced this pull request Feb 5, 2024
@phillebaba @kfcampbell
Add new datasource to get GitHub App token (integrations#1671)
caf6167 
* Add new datasource to get GitHub App token

* Add new datasource to get GitHub App token

* Fix merge error

---------

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
Reviewers

@kfcampbell kfcampbell kfcampbell approved these changes

@bateller bateller bateller approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEAT]: Github App JWT Datasource
3 participants
@phillebaba @bateller @kfcampbell