-
Notifications
You must be signed in to change notification settings - Fork 7
Add GPG Key resource and data source #85
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
base: main
Are you sure you want to change the base?
Conversation
|
I also have a question for the maintainers. Would you be interested in a PR that leverages testcontainers for the acceptance tests, instead of having to manually start a forgejo instance and create a user / token? With that in place, running the tests would be as trivial as running Edit: I went ahead and did it, I wanted to see if it could be done. |
|
Hi @mathieu-lemay, thank you very much for your interest in the provider, and many thanks for your valuable contribution! In fact, GPG keys were on my list for quite a while - I appreciate you helping out! The code looks great! I have some minor comments, but overall you're following all the established patterns - super! Allow me one comment: looking at the SDK docs, you're right in that one can not create keys for other users. However, there's a I'm not entirely sure about the new dependency for creating GPG keys... but I didn't find a better alternative, either. Hence, I appreciate you finding a way to make it work! Let me know what I can do to help... all the best! |
| ID types.Int64 `tfsdk:"id"` | ||
| KeyID types.String `tfsdk:"key_id"` | ||
| PrimaryKeyID types.String `tfsdk:"primary_key_id"` | ||
| Fingerprint types.String `tfsdk:"fingerprint"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is called PublicKey in the Forgejo API... is Fingerprint more accurate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* me doing my best "AI" Agent impression *: You're absolutely right!
I got confused by the fact that what I got back from the API call, and wrongly assumed it was a fingerprint. It was actually the public key itself, the reason it was small is that it was a ed25519 key.
|
|
||
| // gpgKeyDataSourceModel maps the data source schema data. | ||
| // https://pkg.go.dev/codeberg.org/mvdkleijn/forgejo-sdk/forgejo/v2#GPGKey | ||
| type gpgKeyDataSourceModel struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Forgejo API also has Emails and SubsKey... did you omit them deliberately?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to keep the resource simple, based on the (possibly wrong) assumption that most users will just want to use this new resource to save/update their gpg key in forgejo.
I can add them if you think there's a use case for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are absolutely right™ ;-)
I totally agree that we should only add code if there's a realistic use case for it. However, as the Forgejo SDK exposes these additional fields, I think that adding the two attributes comes at virtually no cost, and very little risk. Hence, I would probably add all the argumets that the Forgejo SDK offers, to give Terraform users access to them should the need arise...
But I also agree with your assessment - I trust your judgment and leave it up to you!
|
|
||
| // gpgKeyResourceModel maps the resource schema data. | ||
| // https://pkg.go.dev/codeberg.org/mvdkleijn/forgejo-sdk/forgejo/v2#PublicKey | ||
| type gpgKeyResourceModel struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, you might want to add Emails and SubsKey here...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I trust your judgment and leave it up to you!
| func (r *gpgKeyResource) Schema(_ context.Context, _ resource.SchemaRequest, resp *resource.SchemaResponse) { | ||
| resp.Schema = schema.Schema{ | ||
| Description: `Forgejo user GPG key resource. | ||
| Note: Managing user GPG keys requires administrative privileges!`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really true? You're not using any of the r.client.Admin* functions (targeting the /admin/* endpoints), so I don't think admin privileges are mandatory for this resource.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, it's due to a blind %s/SSH/GPG/g
| } | ||
|
|
||
| // gpgKeyResourceModel maps the resource schema data. | ||
| // https://pkg.go.dev/codeberg.org/mvdkleijn/forgejo-sdk/forgejo/v2#PublicKey |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this should be // https://.../forgejo/v2#GPGKey
internal/provider/provider_test.go
Outdated
| providerConfig = `provider "forgejo" { host = "http://localhost:3000" } | ||
| ` | ||
| providerConfig = `provider "forgejo" { host = "http://localhost:3000" }` | ||
| forgejoEmail = "tfadmin@localhost" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is solely used in gpg_key_resource_test.go then I'd suggest to add it there, and leave provider_test.go unchanged.
|
BTW: GPG supports unattended key generation. Docs here. We should be able to wrap this into a For the examples I'd suggest to simply ingest an armored key as (sensitive) input variable - that's probably the most realistic scenario. |
|
Great feedback, thank you! I'll address that shortly. I'll have to check but I think you're right about not needing admin privileges to edit the current user's key. If we change the data source to read the key of any user, it will require it though. I still think it makes sense for the data source, so I'll change that. |
That might change: https://codeberg.org/forgejo/forgejo/issues/10952. :)
There is https://registry.terraform.io/providers/Poulpatine/pgp/latest/docs/resources/pgp_key? (And https://registry.terraform.io/providers/terraform-provider-gpg/gpg/latest/docs/resources/key_pair, but I guess the passphrase is not always wanted.) |
|
I've addressed all the comments, most of them by actual changes in the code. The only remaining question is that of the emails / subkeys. If you think it would be actually useful for people to expose those, I will add them. For the tests, I went with @kad-hollac1's suggestion of using terraform-provider-gpg. This is much simpler than having to deal with the gpg cli, which doesn't offer an easy way of generating a key to stdout without saving it in the local keyring. Quick note: I've added a bunch of messy commits, but I will rebase into one clean commit once ready to merge. |
acch
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work - thanks much for your hard work @mathieu-lemay. I've added a comment to the remaining item (i.e. additional fields offered by the Forgejo SDK), but I'll leave the decision up to you!
Let me know when you're done and ready to merge. I'd like to tweak a few minor things before finalizing a new release with your changes... from my end we're good to go! 🚀
|
|
||
| # Existing GPG key | ||
| data "forgejo_gpg_key" "this" { | ||
| user = "test_user" # Optional, uses api key's user if not provided |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of ... api key's user ..., let's call this ... authenticated user ... (for consistency).
Sorry for being picky - it's my inner monk ;-)
|
|
||
| # Forgejo GPG key | ||
| resource "forgejo_gpg_key" "this" { | ||
| armored_public_key = gpg_key_pair.test.public_key |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add a comment that there's no user argument here. GPG keys can only be managed for the authenticated user...
|
|
||
| // gpgKeyDataSourceModel maps the data source schema data. | ||
| // https://pkg.go.dev/codeberg.org/mvdkleijn/forgejo-sdk/forgejo/v2#GPGKey | ||
| type gpgKeyDataSourceModel struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are absolutely right™ ;-)
I totally agree that we should only add code if there's a realistic use case for it. However, as the Forgejo SDK exposes these additional fields, I think that adding the two attributes comes at virtually no cost, and very little risk. Hence, I would probably add all the argumets that the Forgejo SDK offers, to give Terraform users access to them should the need arise...
But I also agree with your assessment - I trust your judgment and leave it up to you!
|
|
||
| // gpgKeyResourceModel maps the resource schema data. | ||
| // https://pkg.go.dev/codeberg.org/mvdkleijn/forgejo-sdk/forgejo/v2#PublicKey | ||
| type gpgKeyResourceModel struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I trust your judgment and leave it up to you!
Add a resource and a data source to manage GPG keys. While the forgejo API allows for listing all GPG keys, it is only possible to create / delete them for the current user, so there is no notion of user in those resources.
For testing, I had to bring in a new dependency (github.com/ProtonMail/gopenpgp/v3), as I didn't find a trivial terraform provider to generate random GPG keys.
Note that this PR is incomplete as I have not added examples yet. I wanted to get feedback on this first. If everything looks good, I will add examples.