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

Scaffold APIKeys Key resource #866

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

justinsb
Copy link
Collaborator

@justinsb justinsb commented Sep 27, 2023

Implement the APIKeys Key resource via terraform (which is an oddity in terraform, implemented via DCL).

Also implement a mock.

@justinsb
Copy link
Collaborator Author

This requires isolated imports in mockgcp, as implemented in #863

panic: proto: file "google/api/apikeys/v2/resources.proto" is already registered
	previously from: "cloud.google.com/go/apikeys/apiv2/apikeyspb"
	currently from:  "github.com/GoogleCloudPlatform/k8s-config-connector/mockgcp/generated/google/api/apikeys/v2"
See https://protobuf.dev/reference/go/faq#namespace-conflict

@diviner524
Copy link
Collaborator

@justinsb I may have missed something obvious. This looks similar to how we add a TF based resource. What is the main difference when you mentioned "Manually scaffold"? For example do you plan to manually write the controller for this resource?

@google-oss-prow
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@justinsb
Copy link
Collaborator Author

@diviner524 - sorry, by "manually scaffold" I mean adding support not via the TF autogen approach. I believe that TF autogen doesn't work for this resource because it is DCL based? I should double-check our instructions for how to add a TF resource though, it might answer some of my todos :-)

I do also intend to try implementing this one manually, but first I want to show that the terraform implementation has some problems.

@diviner524
Copy link
Collaborator

@diviner524 - sorry, by "manually scaffold" I mean adding support not via the TF autogen approach. I believe that TF autogen doesn't work for this resource because it is DCL based? I should double-check our instructions for how to add a TF resource though, it might answer some of my todos :-)

I do also intend to try implementing this one manually, but first I want to show that the terraform implementation has some problems.

I see, thanks for sharing!

jsonPath: .status.conditions[?(@.type=='Ready')].lastTransitionTime
name: Status Age
type: date
name: v1alpha1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we are already manually adding it... Why not just mark the CRD as v1beta1? Do you see failing sample tests/integration tests when trying them locally? Or there are some fields with uncertainty?

IIUC this PR is basically doing the same as:

https://github.com/GoogleCloudPlatform/k8s-config-connector/blob/master/README.NewResourceFromTerraform.md

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This resource will only be as good as terraform, I think we have a higher bar for v1beta1? (Maybe we didn't enforce this bar before we introduced v1alpha1?)

@@ -0,0 +1,249 @@
apiVersion: apiextensions.k8s.io/v1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi Justin, is the generation of this CRD purely manual? Or is there some automation involved?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It uses the "classic" generation mechanism (i.e. not automated generation) from a terraform resource

@justinsb justinsb force-pushed the scaffold_apikeys branch 2 times, most recently from ec741ff to c6abf24 Compare January 6, 2024 13:43
@justinsb justinsb changed the title WIP: APIKeys Key resource WIP: Scaffold APIKeys Key resource Jan 6, 2024
@justinsb justinsb force-pushed the scaffold_apikeys branch 3 times, most recently from 40ae646 to a539218 Compare January 6, 2024 14:50
@justinsb justinsb changed the title WIP: Scaffold APIKeys Key resource Scaffold APIKeys Key resource Jan 6, 2024
@justinsb
Copy link
Collaborator Author

justinsb commented Jan 6, 2024

Removing WIP. I believe as long as it's alpha it will be opt-in only.

Change-Id: I6682fcdb74c042c4aae53280472077d417a25d41
This allows us to test our scaffolded API.
@justinsb
Copy link
Collaborator Author

Rebased; I plan on doing the generation again though in case anything changed in terraform in the past 4 months. cc @cheftako

@justinsb
Copy link
Collaborator Author

Ran through generation again, I propose we look at #1168 instead, and then I'll rebase this one and see if there's anything left.

/hold

@yuwenma
Copy link
Collaborator

yuwenma commented Aug 7, 2024

Shall we close this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants