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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: empty returned data field in configmap and secret tables #150

Merged

Conversation

hileef
Copy link
Contributor

@hileef hileef commented Aug 23, 2023

see details in #149

this PR proposes to rename the Data field in the parsedContent struct from Data to ParsedData,
in order to avoid collisions with the Data field from other structs parsedContent is being merged in.


note: it was not clear to me where I could add something to allow for automated tests to ensure no regressions in the future (for example with other possible fields 馃 )

note: tested locally through the make install to validate I was able to retrieve configmap data once more 馃檪

Copy link
Contributor

@bigdatasourav bigdatasourav left a comment

Choose a reason for hiding this comment

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

LGTM

@hileef
Copy link
Contributor Author

hileef commented Aug 23, 2023

Screenshot 2023-08-23 at 13 02 14

Thanks @bigdatasourav ! Is there anything else I can do to help move this forward ? 馃檪 cc. @misraved

@misraved
Copy link
Contributor

misraved commented Aug 23, 2023

@hileef the changes look good 馃憤, however, I was wondering if we could have handled it just by using the Transform function for the data column in kubernetes_secret and kubernetes_config_map tables?

For instance:

In table_kubernetes_secret.go file, could we replace

			{
				Name:        "data",
				Type:        proto.ColumnType_JSON,
				Description: "Contains the secret data.",
			},

with

			{
				Name:        "data",
				Type:        proto.ColumnType_JSON,
				Description: "Contains the secret data.",
				Transform:   transform.FromField("Secret.Data"),
			},

@bigdatasourav thoughts??

@hileef
Copy link
Contributor Author

hileef commented Aug 23, 2023

I was wondering if we could have handled it just by using the Transform function for the data column

Hmmm I could be wrong but my understanding of the code's behaviour on this line

return ConfigMap{*configMap, parsedContent{SourceType: "deployed"}}, nil
is that it will use the empty value of parsedContent.Data to override the ConfigMap.Data field and set its value to nil ( even though it should have been filled in from *configmap.Data which contains the correct value coming from the kube API ),
so unless I'm missing something, I am not sure how the transform function would side-step this 馃

But let's discuss it further, I could be missing something 馃檪 馃檹

@bigdatasourav
Copy link
Contributor

@misraved, Transform function will work in the above scenario; it will map the data column with the Secret.Data value. It will work with both manifest and deployed source types. But should we use Transform in this case and all future cases(if they arise with other upcoming tables)? IMO, we should avoid extra code or patches wherever possible.

@misraved
Copy link
Contributor

Thanks @hileef and @bigdatasourav for the inputs 馃憤.

I think the solution provided by @hileef is a better fit than the Transform function 馃憤.

@misraved misraved merged commit 32f5fd7 into turbot:main Aug 23, 2023
1 check passed
@hileef hileef deleted the fix/defaults/un-hygienic-field-overrides branch August 24, 2023 19:43
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.

None yet

3 participants