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

Gcp cloud functions gen2 flooding #590

Conversation

ashutoshmore658
Copy link
Contributor

@ashutoshmore658 ashutoshmore658 commented May 27, 2024

Fixes #579 issue

@misraved misraved requested a review from ParthaI May 27, 2024 10:24
Copy link
Contributor

@ParthaI ParthaI left a comment

Choose a reason for hiding this comment

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

@ashutoshmore658, please take a look at the review comments. Thanks!

@@ -34,6 +35,7 @@ func tableGcpCloudfunctionFunction(ctx context.Context) *plugin.Table {
Name: "status",
Description: "Status of the function deployment (ACTIVE, OFFLINE, CLOUD_FUNCTION_STATUS_UNSPECIFIED,DEPLOY_IN_PROGRESS, DELETE_IN_PROGRESS, UNKNOWN).",
Type: proto.ColumnType_STRING,
Transform: transform.From(getCloudFunctionStatus),
Copy link
Contributor

Choose a reason for hiding this comment

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

@ashutoshmore658, is a separate transform function necessary? Wouldn't transform.FromField("State") suffice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No it would not suffice, I have checked it, but if you say, I will check it again.
Thank you.

Copy link
Contributor

@ParthaI ParthaI May 29, 2024

Choose a reason for hiding this comment

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

Hello @ashutoshmore658, I've taken another look at the changes made to this table. I can confirm that, using the default transform function transform.FromField("State") is good for us, and there's no need to define a separate transform function for populating the column value.

Here is the result with the default transform function.

> select name, status from gcp_cloudfunctions_function
+------------------+--------+
| name             | status |
+------------------+--------+
| function-1-gen-2 | ACTIVE |
| function-1-gen-1 | ACTIVE |

Is there any special case where we are not getting the status column value with the default transform function?

Copy link
Contributor

@ParthaI ParthaI left a comment

Choose a reason for hiding this comment

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

@ashutoshmore658, I've made some additional observations, please have a look:

  • The column values for runtime, available_memory_mb, build_id, entry_point, ingress_settings, max_instances, service_account_email, timeout, vpc_connector, and vpc_connector_egress_settings can be populated using the default transform function.
  • The available_memory_mb column should be of type string, rather than int.
  • For the columns source_upload_url and https_trigger, a separate transform function may be necessary, as the API does not return these properties directly.
  • Considering we are uncertain of how users are utilizing this table in their setups, it might be prudent to refrain from modifying existing column values. Instead, adding new columns based on the API responses we receive could be more beneficial.

Please feel free to reach out if you have any questions or need further clarification.

Thank you!

@@ -34,6 +35,7 @@ func tableGcpCloudfunctionFunction(ctx context.Context) *plugin.Table {
Name: "status",
Description: "Status of the function deployment (ACTIVE, OFFLINE, CLOUD_FUNCTION_STATUS_UNSPECIFIED,DEPLOY_IN_PROGRESS, DELETE_IN_PROGRESS, UNKNOWN).",
Type: proto.ColumnType_STRING,
Transform: transform.From(getCloudFunctionStatus),
Copy link
Contributor

@ParthaI ParthaI May 29, 2024

Choose a reason for hiding this comment

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

Hello @ashutoshmore658, I've taken another look at the changes made to this table. I can confirm that, using the default transform function transform.FromField("State") is good for us, and there's no need to define a separate transform function for populating the column value.

Here is the result with the default transform function.

> select name, status from gcp_cloudfunctions_function
+------------------+--------+
| name             | status |
+------------------+--------+
| function-1-gen-2 | ACTIVE |
| function-1-gen-1 | ACTIVE |

Is there any special case where we are not getting the status column value with the default transform function?

@ashutoshmore658
Copy link
Contributor Author

ashutoshmore658 commented Jun 1, 2024

@ParthaI sure, let me take a look at this. Thank you for the valuable feedback.
also let me do a POC with the changes you are suggesting, because before going this way I tried with your approach but it was populating null values for these columns. So I decided to code by this way.

@ashutoshmore658
Copy link
Contributor Author

@ParthaI actually I have tried by modifying code, checked with multiple times restarting the system(to avoid cached builds),
I am getting null for the columns,
it is working for the status only.
please have a look at the following test screenshot
Screenshot from 2024-06-02 03-47-48

@ParthaI
Copy link
Contributor

ParthaI commented Jun 3, 2024

Hello, @ashutoshmore658, I am not sure what the issue is with your system. I can see the result for all of the columns as mentioned here.

Screenshot 2024-06-03 at 10 40 42 AM

Note: I have renamed the column environment_variables to service_environment_variables to distinguish between the two types of environment variables: one for build and one for service. I am not retrieving the build_environment_variables because I have not configured it.

I have pushed the tested code to the branch test-gcp-cloud-function-table-column. Please feel free to take a look.

Thanks!

@ashutoshmore658
Copy link
Contributor Author

Sure @ParthaI , I will test and lets merge it if it looks fine and okay to merge.

@ParthaI
Copy link
Contributor

ParthaI commented Jun 21, 2024

Hi @ashutoshmore658, Did you get a chance to retake a look at it?

@misraved misraved changed the base branch from main to fix-gcp-cloud-functions-missing-columns July 9, 2024 05:38
@misraved misraved merged commit 1ab0634 into turbot:fix-gcp-cloud-functions-missing-columns Jul 9, 2024
1 check passed
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.

gcp_cloudfunctions_function is missing some of the column values.
4 participants