Skip to content
This repository was archived by the owner on May 6, 2026. It is now read-only.

Split apart cloudNetwork device attribute#83

Merged
aojea merged 1 commit into
google:mainfrom
ngcxy:split-attributes
May 29, 2025
Merged

Split apart cloudNetwork device attribute#83
aojea merged 1 commit into
google:mainfrom
ngcxy:split-attributes

Conversation

@ngcxy
Copy link
Copy Markdown
Contributor

@ngcxy ngcxy commented May 22, 2025

Improve attribute readability for cloud network and flexibility for cloud provider.
This is a draft PR that only contains the key logic, without comprehensive case handling.

@BenTheElder BenTheElder requested a review from aojea May 22, 2025 02:11
@gauravkghildiyal gauravkghildiyal requested review from gauravkghildiyal and removed request for aojea May 22, 2025 03:01
Comment thread pkg/inventory/cloud.go Outdated
Comment thread pkg/inventory/cloud.go Outdated
Comment thread pkg/inventory/db.go Outdated
Comment thread pkg/cloudprovider/gce/gce.go Outdated
Comment thread pkg/cloudprovider/gce/gce.go Outdated
Comment thread pkg/cloudprovider/cloud.go Outdated
@aojea aojea marked this pull request as ready for review May 23, 2025 07:13
Comment thread pkg/cloudprovider/cloud.go Outdated
Comment thread pkg/cloudprovider/gce/gce.go Outdated
Comment thread pkg/inventory/cloud.go Outdated
Comment thread pkg/inventory/cloud.go Outdated
Copy link
Copy Markdown
Member

@gauravkghildiyal gauravkghildiyal left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @ngcxy ! This is great to start with.

I've made a few comments. As you become more familiar with these things, these will get more natural to you.

Comment thread pkg/cloudprovider/gce/gce.go
Comment thread pkg/inventory/cloud.go Outdated
Comment thread pkg/inventory/cloud.go Outdated
Comment thread pkg/cloudprovider/gce/gce.go
Comment thread pkg/inventory/db.go Outdated
Comment thread pkg/inventory/db.go Outdated
Comment thread pkg/inventory/cloud_test.go Outdated
Comment thread pkg/inventory/cloud_test.go Outdated
Comment thread pkg/cloudprovider/cloud.go Outdated
@gauravkghildiyal gauravkghildiyal self-assigned this May 27, 2025
@aojea
Copy link
Copy Markdown
Contributor

aojea commented May 28, 2025

overal LGTM. @gauravkghildiyal for final decision, ping me for merging

Copy link
Copy Markdown
Member

@gauravkghildiyal gauravkghildiyal left a comment

Choose a reason for hiding this comment

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

Almost there

Comment thread pkg/inventory/cloud.go Outdated
Comment thread pkg/inventory/cloud.go Outdated
Comment thread pkg/inventory/db.go Outdated
Comment thread pkg/cloudprovider/gce/gce.go Outdated
Comment thread pkg/inventory/cloud_test.go
The initial attribute of "cloudNetwork" contains a string with both the
network name and the project number, which may exceed the limitation of
attribute size.
This commit splits the "cloudNetwork" into two parts, as well as
introduces provider-specific function to manage the attributes.
Copy link
Copy Markdown
Member

@gauravkghildiyal gauravkghildiyal left a comment

Choose a reason for hiding this comment

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

LGTM.
Ready to merge @aojea

Copy link
Copy Markdown
Contributor

@aojea aojea left a comment

Choose a reason for hiding this comment

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

Thanks

@aojea aojea merged commit 8b881ed into google:main May 29, 2025
6 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants