Skip to content

Adding functionality to check if GPG keys are valid for signing providers #47

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

Closed
wants to merge 99 commits into from

Conversation

diofeher
Copy link
Member

@diofeher diofeher commented Jan 9, 2025

This code is used by registry repo to validate if a key was used to sign a provider.

It's going to be used on registry. It closes opentofu/registry#1423

Signed-off-by: Diogenes Fernandes <[email protected]>
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Thank you for the work @diofeher. There are a few things that would need to be changed here. Mainly, this shouldn't be part of the metadata API. Also, this PR seems to be lacking tests.

@diofeher
Copy link
Member Author

diofeher commented Jan 9, 2025

Thanks for your feedback @abstractionfactory ! This PR was meant to be a draft, didn't see it was a normal PR, sorry :( I'm still testing with the another repo....
But it's good that I received feedback already, gonna fix these issues.

@diofeher diofeher marked this pull request as draft January 9, 2025 18:32
Signed-off-by: Diogenes Fernandes <[email protected]>
Signed-off-by: Diogenes Fernandes <[email protected]>
@cam72cam
Copy link
Member

I think @abstractionfactory has been looking forward to getting this fixed for a while :)

Signed-off-by: Diogenes Fernandes <[email protected]>
Signed-off-by: Diogenes Fernandes <[email protected]>
Signed-off-by: Diogenes Fernandes <[email protected]>
Signed-off-by: Diogenes Fernandes <[email protected]>
Signed-off-by: Diogenes Fernandes <[email protected]>
Signed-off-by: Diogenes Fernandes <[email protected]>
@ghost
Copy link

ghost commented Jan 10, 2025

Have I ever! :D Thanks for all your hard work @diofeher !

Signed-off-by: Diogenes Fernandes <[email protected]>
Signed-off-by: Diogenes Fernandes <[email protected]>
@diofeher diofeher requested a review from a user January 10, 2025 15:55
@diofeher diofeher marked this pull request as ready for review January 10, 2025 15:56
@ghost ghost self-assigned this Jan 10, 2025
Signed-off-by: Diogenes Fernandes <[email protected]>
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Hi @diofeher thank you very much for your work!

diofeher and others added 6 commits January 13, 2025 09:43
Co-authored-by: AbstractionFactory <[email protected]>
Signed-off-by: Diógenes Fernandes <[email protected]>
Co-authored-by: AbstractionFactory <[email protected]>
Signed-off-by: Diógenes Fernandes <[email protected]>
Co-authored-by: AbstractionFactory <[email protected]>
Signed-off-by: Diógenes Fernandes <[email protected]>
Signed-off-by: Diogenes Fernandes <[email protected]>
Signed-off-by: Diógenes Fernandes <[email protected]>
Signed-off-by: Diógenes Fernandes <[email protected]>
Signed-off-by: Diogenes Fernandes <[email protected]>
Copy link

@yhakbar yhakbar left a comment

Choose a reason for hiding this comment

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

Most of my comments are NITs.

Given that I'm not a maintainer of the project, I won't give explicit approval. I don't see any major issues, though.

Copy link

@Resonance1584 Resonance1584 left a comment

Choose a reason for hiding this comment

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

Looks really good overall! I have one comment around if the gopenpgp package is thread safe

Signed-off-by: Diógenes Fernandes <[email protected]>
Signed-off-by: Diógenes Fernandes <[email protected]>
Signed-off-by: Diógenes Fernandes <[email protected]>
Signed-off-by: Diógenes Fernandes <[email protected]>
Signed-off-by: Diógenes Fernandes <[email protected]>
Signed-off-by: Diógenes Fernandes <[email protected]>
Signed-off-by: Diógenes Fernandes <[email protected]>
Signed-off-by: Diógenes Fernandes <[email protected]>
Signed-off-by: Diógenes Fernandes <[email protected]>
Signed-off-by: Diógenes Fernandes <[email protected]>
@diofeher diofeher requested a review from a user February 13, 2025 12:58
@diofeher diofeher marked this pull request as ready for review February 13, 2025 12:58
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

This is looking a lot better as far as tests and code intent is concerned, but it looks like the public signature has now switched to being an abstraction for the provider key itself, which was not the intention. If that was the case, this package would be in the wrong place (check where the provider types are).

Also, it is worth mentioning that mixing value types and tools/services in one data structure makes keeping the code clean tricky, because tools have dependencies (such as the metadata API) and it becomes had to not bloat the instantiation of the value-type with dependencies. It's a better idea to separate value types and services into separate entities.

Signed-off-by: Diogenes Fernandes <[email protected]>
Signed-off-by: Diogenes Fernandes <[email protected]>
Signed-off-by: Diogenes Fernandes <[email protected]>
@ghost ghost removed their assignment Feb 18, 2025
Signed-off-by: Diógenes Fernandes <[email protected]>
@Yantrio Yantrio marked this pull request as draft March 3, 2025 15:49
@Yantrio
Copy link
Member

Yantrio commented Mar 3, 2025

I've moved this to draft so that we can discuss this more amongst the opentofu team.

@diofeher
Copy link
Member Author

Closing this one in favor of opentofu/registry#1423

@diofeher diofeher closed this Apr 23, 2025
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.

6 participants