Ftr logging link alpha #3181

tylerreidwaze · 2024-11-14T19:50:15Z

Change description

This PR adds LoggingLinks as a KCC Resource

Tests you have done

Run make ready-pr to ensure this PR is ready for review.
Perform necessary E2E testing for changed resources.

apis/logging/v1alpha1/link_types.go

apis/refs/v1beta1/logbucketref.go

pkg/clients/generated/apis/logging/v1alpha1/doc.go

maqiuyujoyce

Hi @tylerreidwaze , thank you so much for adding the resource.

before you addressing the comments, could you rebase to master HEAD and solve the conflict?

apis/refs/v1beta1/logbucketref.go

apis/logging/v1alpha1/link_identity.go

apis/logging/v1alpha1/link_reference.go

pkg/controller/direct/logging/link_controller.go

maqiuyujoyce · 2025-03-13T01:14:24Z

pkg/controller/direct/logging/link_controller.go

+	req := &loggingpb.DeleteLinkRequest{Name: a.id.External}
+	op, err := a.gcpClient.DeleteLink(ctx, req)
+	if err != nil {
+		return false, fmt.Errorf("deleting Link %s: %w", a.id.External, err)


A special handling for NotFound error would be useful here.

Just a note or something to implement here?

Addressed in the new comment: #3181 (comment)

...logging/v1alpha1/logginglogginglink/basiclink/_generated_object_projectlogbucket.golden.yaml

maqiuyujoyce · 2025-03-13T01:23:33Z

...est/resourcefixture/testdata/basic/logging/v1alpha1/logginglogginglink/basiclink/create.yaml

+apiVersion: logging.cnrm.cloud.google.com/v1alpha1
+kind: LoggingLink
+metadata:
+  name: logginglink${uniqueId} # no hyphen here as link ids can only have alphanumeric and underscores


If no hyphen is supported in Logging link resource, I suggest you use spec.resourceID to specify the GCP resource ID, and add the hyphen back here. One reason we introduced spec.resourceID was to solve the problem about inconsistent naming conventions between K8s objects and GCP resources.

Now this is fixed, found a bug with this so good call :)

...est/resourcefixture/testdata/basic/logging/v1alpha1/logginglogginglink/basiclink/create.yaml

pkg/controller/direct/logging/link_controller.go

maqiuyujoyce

Thank you for the addressing the comments, @tylerreidwaze !

You'll need to rebase against master HEAD again due to the new merge conflict.

Also, have you got the chance to run the tests locally against the real GCP API after you've made the change?

apis/logging/v1alpha1/link_identity.go

maqiuyujoyce · 2025-03-20T02:21:11Z

apis/logging/v1alpha1/link_types.go

+// LoggingLinkSpec defines the desired state of LoggingLink
+// +kcc:proto=google.logging.v2.Link
+type LoggingLinkSpec struct {
+	// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="ResourceID field is immutable"


For the "incoming commit", did you mean a separate PR or did I miss anything?

maqiuyujoyce · 2025-03-20T02:22:01Z

apis/logging/v1alpha1/link_types.go

+	//  The maximum length of the description is 8000 characters.
+	Description *string `json:"description,omitempty"`
+
+	// Placeholder description


apis/logging/v1alpha1/link_types.go

...est/resourcefixture/testdata/basic/logging/v1alpha1/logginglogginglink/basiclink/create.yaml

pkg/controller/direct/logging/link_controller.go

tylerreidwaze

I see the issue. I didn't save a few files in vscode and thus the changes werent uploaded

tylerreidwaze · 2025-03-20T15:22:49Z

apis/logging/v1alpha1/link_types.go

+	//  The maximum length of the description is 8000 characters.
+	Description *string `json:"description,omitempty"`
+
+	// Placeholder description


Fix in most recent, not sure what happened here.

tylerreidwaze · 2025-03-20T15:23:00Z

apis/logging/v1alpha1/link_types.go

+// LoggingLinkSpec defines the desired state of LoggingLink
+// +kcc:proto=google.logging.v2.Link
+type LoggingLinkSpec struct {
+	// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="ResourceID field is immutable"


I am not sure what happened here. Somehow this wasnt added to a commit. Anywho, its fixed now

tylerreidwaze · 2025-03-20T15:23:20Z

apis/logging/v1alpha1/link_types.go

+type LoggingLinkObservedState struct {
+
+	// +optional
+	// +kubebuilder:validation:Format=date-time


Another one that was lost, fixed.

apis/logging/v1alpha1/link_types.go

pkg/controller/direct/logging/link_controller.go

apis/refs/v1beta1/logbucketref.go

tylerreidwaze · 2025-03-20T21:20:30Z

Tests have been run locally and are passing against real GCP. Rebase against master complete. Sorry for the issues and confusing commits I had in here

maqiuyujoyce

LGTM except for a small issue.

Also the validations failed so could you rerun make ready-pr?

apis/logging/v1alpha1/link_identity.go

pkg/controller/direct/logging/link_controller.go

real.txt

github-advanced-security · 2025-03-21T17:18:43Z

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

tylerreidwaze

Getting closer :)

apis/logging/v1alpha1/link_identity.go

pkg/controller/direct/logging/link_controller.go

real.txt

maqiuyujoyce

/lgtm
/approve

google-oss-prow · 2025-03-21T21:49:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maqiuyujoyce

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:

~~OWNERS~~ [maqiuyujoyce]

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

tylerreidwaze mentioned this pull request Nov 14, 2024

WIP: feat: working on onboarding logging links for kcc #3014

Closed

2 tasks

tylerreidwaze changed the title ~~Ftr logging link alpha~~ do-not-merge/work-in-progress - Ftr logging link alpha Nov 15, 2024

tylerreidwaze added the do-not-merge/work-in-progress label Nov 15, 2024

yuwenma reviewed Feb 4, 2025

View reviewed changes

apis/logging/v1alpha1/link_types.go Outdated Show resolved Hide resolved

apis/refs/v1beta1/logbucketref.go Show resolved Hide resolved

pkg/clients/generated/apis/logging/v1alpha1/doc.go Outdated Show resolved Hide resolved

google-oss-prow bot removed the do-not-merge/work-in-progress label Feb 6, 2025

tylerreidwaze force-pushed the ftr-logging-link-alpha branch from 1b8855d to 752fc03 Compare February 6, 2025 22:53

tylerreidwaze changed the title ~~do-not-merge/work-in-progress - Ftr logging link alpha~~ Ftr logging link alpha Feb 7, 2025

tylerreidwaze added the enhancement New feature or request label Feb 10, 2025

tylerreidwaze force-pushed the ftr-logging-link-alpha branch 2 times, most recently from ed24352 to 5b13a70 Compare February 26, 2025 22:55

maqiuyujoyce reviewed Mar 13, 2025

View reviewed changes

tylerreidwaze force-pushed the ftr-logging-link-alpha branch 4 times, most recently from c0db853 to 67766fe Compare March 14, 2025 22:37

maqiuyujoyce reviewed Mar 20, 2025

View reviewed changes

tylerreidwaze force-pushed the ftr-logging-link-alpha branch from a38fe82 to 7c7aeef Compare March 20, 2025 15:14

tylerreidwaze commented Mar 20, 2025

View reviewed changes

tylerreidwaze force-pushed the ftr-logging-link-alpha branch 5 times, most recently from 5153394 to ec8bd77 Compare March 20, 2025 21:19

tylerreidwaze force-pushed the ftr-logging-link-alpha branch from ec8bd77 to a0074b3 Compare March 20, 2025 21:21

maqiuyujoyce reviewed Mar 21, 2025

View reviewed changes

apis/logging/v1alpha1/link_identity.go Show resolved Hide resolved

pkg/controller/direct/logging/link_controller.go Outdated Show resolved Hide resolved

real.txt Outdated Show resolved Hide resolved

tylerreidwaze force-pushed the ftr-logging-link-alpha branch from a0074b3 to 98517d5 Compare March 21, 2025 17:17

tylerreidwaze commented Mar 21, 2025

View reviewed changes

apis/logging/v1alpha1/link_identity.go Show resolved Hide resolved

pkg/controller/direct/logging/link_controller.go Outdated Show resolved Hide resolved

real.txt Outdated Show resolved Hide resolved

feat: implement logginglink

7d54598

wTylerReid added 20 commits March 21, 2025 17:25

remove some todos

2c208df

fix issues with go.mod

d7e8a70

update go mod and sum

877b0b7

cleanup comments and unused files

f3dbf32

remove mock changes since we are not implementing in alpha

5209c6e

respond to PR comments, fix all the non code changes

c127e01

change test data to use resourceID

eb8ca41

removing hyphen that worked its way back in

4fad19a

add handling for update method

fec9a34

use string method and reduce references to External

ee0202e

update to use LinkIdentity, remove reference to unused NewLoggingLinkRef

92806d6

correct log bucket ID in LinkIdentity

8e7de32

rerun make ensure

47a3ae5

formatting and logging cleanups

89ff74f

a few more small fixes I forgot to push

53fe8c3

correct unused variable, correct string method

555d23b

add case for delete when there is a 404

0674f29

one last make enforce run

5920710

check resource id, check response on wait

48e51b0

rerun generators

76b55f1

tylerreidwaze force-pushed the ftr-logging-link-alpha branch from 98517d5 to 76b55f1 Compare March 21, 2025 18:26

tylerreidwaze enabled auto-merge March 21, 2025 20:55

maqiuyujoyce reviewed Mar 21, 2025

View reviewed changes

google-oss-prow bot assigned maqiuyujoyce Mar 21, 2025

google-oss-prow bot added the lgtm label Mar 21, 2025

google-oss-prow bot added the approved label Mar 21, 2025

google-oss-prow bot merged commit de12150 into master Mar 21, 2025
29 checks passed

Ftr logging link alpha #3181

Ftr logging link alpha #3181

Uh oh!

Conversation

tylerreidwaze commented Nov 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Change description

Tests you have done

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maqiuyujoyce left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

maqiuyujoyce left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tylerreidwaze left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tylerreidwaze commented Mar 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maqiuyujoyce left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-advanced-security bot commented Mar 21, 2025

Uh oh!

tylerreidwaze left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maqiuyujoyce left a comment

Choose a reason for hiding this comment

Uh oh!

tylerreidwaze commented Nov 14, 2024 •

edited

Loading

tylerreidwaze commented Mar 20, 2025 •

edited

Loading