-
Notifications
You must be signed in to change notification settings - Fork 239
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
direct: Generate crd and upload status.externalRef #2599
Conversation
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.
/lgtm
/approve
/hold
one minor but no blockers. Feel free to resolve in follow up PRs.
dev/tasks/generate-crds
Outdated
@@ -55,7 +55,7 @@ go run ./scripts/crd-tools reflow-descriptions --dir apis/config/crd/ | |||
|
|||
# excluded_resources are resources that are under development for a direct conversion | |||
# we don't modify the CRD just yet for those but will in the future | |||
excluded_resources=("computeforwardingrule") | |||
#excluded_resources=("computeforwardingrule") |
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 the resource is ready, maybe not place it inside excluded_resources
just in case it's turned back off by accident?
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.
Thanks! I made this a part of the comment, so people have an example on how to use this flag when working on direct controllers.
# excluded_resources are resources that are under development for a direct conversion
# we don't modify the CRD just yet for those but will in the future
# i.e. excluded_resources=("computeforwardingrule")
edit: I removed "computeforwardingrule" from excluded_resources
@@ -53,14 +53,14 @@ func asID(externalRef string) (*ForwardingRuleIdentity, error) { | |||
} | |||
path := strings.TrimPrefix(externalRef, serviceDomain+"/") | |||
tokens := strings.Split(path, "/") | |||
if len(tokens) == 5 || tokens[0] == "projects" || tokens[2] == "global" || tokens[3] == "forwardingrules" { | |||
if len(tokens) == 5 && tokens[0] == "projects" && tokens[2] == "global" && tokens[3] == "forwardingrules" { |
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!
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: yuwenma 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 |
a659a9b
to
483faf7
Compare
description: 'Allowed value: The `address` field of a `ComputeAddress` | ||
resource.' | ||
description: The ComputeAddress selflink in the form "projects/{{project}}/regions/{{region}}/addresses/{{name}}" | ||
when not managed by KCC. |
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.
nit: managed by KCC
--> managed by ConfigConnector
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.
/lgtm
3747a07
into
GoogleCloudPlatform:master
Change description
Generated CRD and populate status.externalRef in golden log
Tests you have done
make ready-pr
to ensure this PR is ready for review.