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

Add support for GitHub app credentials #235

Merged
merged 15 commits into from
Mar 14, 2023

Conversation

philamente
Copy link
Contributor

We need to switch our authentication towards github to GitHub App. As there was also a feature request for this one in the issues, I hence added GitHub App functionality to the following ressources:

  • argocd_repository
  • argocd_respository_credentials

The underlying package already supports it

@onematchfox onematchfox self-requested a review March 5, 2023 17:03
Copy link
Collaborator

@onematchfox onematchfox left a comment

Choose a reason for hiding this comment

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

Hi @philamente,

Thanks for the contribution!

There are a few things that need to be addressed before we can merge this.

  1. I see you have added these fields to both the structure of repository and repository_credentials but have only added them to the schema of repository_credentials (and not repository). Was this intentional?
  2. Please can you ensure that you add tests covering the population of the newly added attributes to the affected resources? Note: at present, the acceptance tests are failing, so this will also need to be addressed, although I think this will be resolved by point 1.
  3. As GithubAppId and GithubInstallationId are int64 types within ArgoCD, I believe you will need to set these to schema.TypeString within the Terraform schema (rather than schema.TypeInt) and then convert between string/int64 as appropriate. This can be tested/validated by testing the max boundary of these values in the test you add in point 2. For reference, see the shard attribute on the argocd_cluster resource and the usage of the utils function convertStringToInt64 within this repository.
  4. Docs will need to be updated based to reflect changes in point 3.

Feel free to reach out if anything I've said is unclear and/or if you need more specific guidance!

@philamente
Copy link
Contributor Author

Hi @onematchfox,

thanks for your kind feedback! I saw the failing tests from github actions - unfortunately my local test setup did not yet work properly (working on a corporate pc with annoying limitations - might switch to private on for testing env).

This contribution is my first attempt in Go, so thanks for baring with me and providing such a detailed feedback. ;) I also noticed that we might have an issue with one of the maps which were set to String only and hence will break with int. Your proposal in 3) could circumvent that.

Thanks!

@philamente
Copy link
Contributor Author

@onematchfox Maybe you have an idea: the two ids (github_app as well as the github_app_installation) require type int64. Hence using the Int64Pointer does not work/make sense.

However through the conversion to String the optional (unset) case of 0 does not get handled as null by terraform as it gets parsed to "0".

--> Right now I am thinking about the following:

  1. Inserting a modified intToString conversion in .utils that handles that optional 0 case

But most likely I miss something here, so maybe you have an even better/more elegant solution

image

@onematchfox
Copy link
Collaborator

Going to go ahead and merge this now. Thanks, @philamente, for the contribution!

@onematchfox onematchfox changed the title Adding support for github app as repo credential Add support for GitHub app credentials Mar 14, 2023
@onematchfox onematchfox merged commit cb9f554 into argoproj-labs:master Mar 14, 2023
@philamente
Copy link
Contributor Author

Going to go ahead and merge this now. Thanks, @philamente, for the contribution!

Thank you very much for feedback and review!

MrLuje pushed a commit to MrLuje/terraform-provider-argocd that referenced this pull request Apr 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants