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

Adding a fix for nil pointer issue in converter. #5742

Merged
merged 4 commits into from
Jun 16, 2023

Conversation

vishwahiremat
Copy link
Contributor

@vishwahiremat vishwahiremat commented Jun 16, 2023

Description

Added changes to throw an error if conversion fails.

Issue reference

Fixes: #issue_number

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • Adds necessary unit tests for change
  • Adds necessary E2E tests for change
  • Unit tests passing
  • Extended the documentation / Created issue for it

Auto-generated summary

🤖 Generated by Copilot at 495ce78

Summary

🐛🔧🚧

This pull request adds error handling to various data model conversion functions in the pkg/linkrp/datamodel/converter package. These functions are used to transform versioned data models of different resources to non-versioned ones for the LinkRP service.

DataModelFromVersioned
Converts with error checks
Fall leaves may wither

Walkthrough

@vishwahiremat vishwahiremat requested a review from a team as a code owner June 16, 2023 18:32
@github-actions
Copy link

github-actions bot commented Jun 16, 2023

Radius functional test overview

🔍 Go to test action run

Name Value
Repository project-radius/radius
Commit ref 495ce78
Unique ID 05cacde4c5
Image tag pr-05cacde4c5
Click here to see the list of tools in the current test run
  • gotestsum 1.10.0
  • KinD: v0.18.0
  • Dapr: 1.10.0
  • Azure KeyVault CSI driver: 1.4.2
  • Azure Workload identity webhook: 1.0.0
  • recipe location radiusdev.azurecr.io/test/functional/corerp/recipes/<name>:pr-05cacde4c5
  • appcore-rp test image location: radiusdev.azurecr.io/appcore-rp:pr-05cacde4c5
  • ucp test image location: radiusdev.azurecr.io/ucpd:pr-05cacde4c5

Test Status

⌛ Building Radius and pushing container images for functional tests...
✅ Container images build succeeded
⌛ Publishing Bicep Recipes for functional tests...
✅ Recipe publishing succeeded
⌛ Starting samples functional tests...
⌛ Starting corerp functional tests...
⌛ Starting ucp functional tests...
✅ samples functional tests succeeded
✅ ucp functional tests succeeded
✅ corerp functional tests succeeded

@@ -46,6 +46,9 @@ func SqlDatabaseDataModelFromVersioned(content []byte, version string) (*datamod
return nil, err
}
dm, err := am.ConvertTo()
if err != nil {
return nil, err
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for checking the others. I was about to ask.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have tests that cover these cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for pointing this out, the invalid tests we had were not covering this case. added tests for this case

@github-actions
Copy link

github-actions bot commented Jun 16, 2023

Radius functional test overview

🔍 Go to test action run

Name Value
Repository project-radius/radius
Commit ref 2b5d5d9
Unique ID e49e771387
Image tag pr-e49e771387
Click here to see the list of tools in the current test run
  • gotestsum 1.10.0
  • KinD: v0.18.0
  • Dapr: 1.10.0
  • Azure KeyVault CSI driver: 1.4.2
  • Azure Workload identity webhook: 1.0.0
  • recipe location radiusdev.azurecr.io/test/functional/corerp/recipes/<name>:pr-e49e771387
  • appcore-rp test image location: radiusdev.azurecr.io/appcore-rp:pr-e49e771387
  • ucp test image location: radiusdev.azurecr.io/ucpd:pr-e49e771387

Test Status

⌛ Building Radius and pushing container images for functional tests...
✅ Container images build succeeded
⌛ Publishing Bicep Recipes for functional tests...
✅ Recipe publishing succeeded
⌛ Starting corerp functional tests...
⌛ Starting ucp functional tests...
⌛ Starting samples functional tests...
✅ samples functional tests succeeded
✅ ucp functional tests succeeded
✅ corerp functional tests succeeded

@github-actions
Copy link

github-actions bot commented Jun 16, 2023

Test Results

2 719 tests  +5   2 712 ✔️ +5   2m 1s ⏱️ -1s
   240 suites ±0          7 💤 ±0 
       1 files   ±0          0 ±0 

Results for commit 2536682. ± Comparison against base commit bfcef12.

This pull request removes 2 and adds 7 tests. Note that renamed tests count towards both.
github.com/project-radius/radius/pkg/armrpc/frontend/controller ‑ TestValidateEtag_IfMatch/170f7aaf-632a-4497-912f-9502fc15e169
github.com/project-radius/radius/pkg/armrpc/frontend/controller ‑ TestValidateEtag_IfMatch/170f7aaf-632a-4497-912f-9502fc15e169#01
github.com/project-radius/radius/pkg/armrpc/frontend/controller ‑ TestValidateEtag_IfMatch/729effa1-7e2c-48db-8c68-381177f1b949
github.com/project-radius/radius/pkg/armrpc/frontend/controller ‑ TestValidateEtag_IfMatch/729effa1-7e2c-48db-8c68-381177f1b949#01
github.com/project-radius/radius/pkg/linkrp/datamodel/converter ‑ TestDaprSecretStoreDataModelFromVersioned/2022-03-15-privatepreview#02
github.com/project-radius/radius/pkg/linkrp/datamodel/converter ‑ TestMongoDatabaseDataModelFromVersioned/2022-03-15-privatepreview#02
github.com/project-radius/radius/pkg/linkrp/datamodel/converter ‑ TestRabbitMQMessageQueueDataModelFromVersioned/2022-03-15-privatepreview#02
github.com/project-radius/radius/pkg/linkrp/datamodel/converter ‑ TestRedisCacheDataModelFromVersioned/2022-03-15-privatepreview#02
github.com/project-radius/radius/pkg/linkrp/datamodel/converter ‑ TestSqlDatabaseDataModelFromVersioned/2022-03-15-privatepreview#03

♻️ This comment has been updated with latest results.

@github-actions
Copy link

63.9

For the detailed report, please go to Checks tab, click Build and Test, and then download unit_test_coverage artifact at the bottom of build page.

  • Your PR branch coverage: 63.9 %
  • main branch coverage: 63.9 %
  • diff coverage: 0 %

The coverage result does not include the functional test coverage.

@github-actions
Copy link

github-actions bot commented Jun 16, 2023

Radius functional test overview

🔍 Go to test action run

Name Value
Repository project-radius/radius
Commit ref 548b61e
Unique ID baeef3e28f
Image tag pr-baeef3e28f
Click here to see the list of tools in the current test run
  • gotestsum 1.10.0
  • KinD: v0.18.0
  • Dapr: 1.10.0
  • Azure KeyVault CSI driver: 1.4.2
  • Azure Workload identity webhook: 1.0.0
  • recipe location radiusdev.azurecr.io/test/functional/corerp/recipes/<name>:pr-baeef3e28f
  • appcore-rp test image location: radiusdev.azurecr.io/appcore-rp:pr-baeef3e28f
  • ucp test image location: radiusdev.azurecr.io/ucpd:pr-baeef3e28f

Test Status

⌛ Building Radius and pushing container images for functional tests...
✅ Container images build succeeded
⌛ Publishing Bicep Recipes for functional tests...
✅ Recipe publishing succeeded
⌛ Starting ucp functional tests...
⌛ Starting samples functional tests...
✅ samples functional tests succeeded
✅ ucp functional tests succeeded
✅ corerp functional tests succeeded

@github-actions
Copy link

64.0

For the detailed report, please go to Checks tab, click Build and Test, and then download unit_test_coverage artifact at the bottom of build page.

  • Your PR branch coverage: 64.0 %
  • main branch coverage: 63.9 %
  • diff coverage: .1 %

The coverage result does not include the functional test coverage.

@github-actions
Copy link

github-actions bot commented Jun 16, 2023

Radius functional test overview

🔍 Go to test action run

Name Value
Repository project-radius/radius
Commit ref 2536682
Unique ID dd320900c1
Image tag pr-dd320900c1
Click here to see the list of tools in the current test run
  • gotestsum 1.10.0
  • KinD: v0.18.0
  • Dapr: 1.10.0
  • Azure KeyVault CSI driver: 1.4.2
  • Azure Workload identity webhook: 1.0.0
  • recipe location radiusdev.azurecr.io/test/functional/corerp/recipes/<name>:pr-dd320900c1
  • appcore-rp test image location: radiusdev.azurecr.io/appcore-rp:pr-dd320900c1
  • ucp test image location: radiusdev.azurecr.io/ucpd:pr-dd320900c1

Test Status

⌛ Building Radius and pushing container images for functional tests...
✅ Container images build succeeded
⌛ Publishing Bicep Recipes for functional tests...
✅ Recipe publishing succeeded
⌛ Starting samples functional tests...
⌛ Starting corerp functional tests...
⌛ Starting ucp functional tests...
✅ samples functional tests succeeded
✅ ucp functional tests succeeded
✅ corerp functional tests succeeded

@github-actions
Copy link

64.0

For the detailed report, please go to Checks tab, click Build and Test, and then download unit_test_coverage artifact at the bottom of build page.

  • Your PR branch coverage: 64.0 %
  • main branch coverage: 63.9 %
  • diff coverage: .1 %

The coverage result does not include the functional test coverage.

@vishwahiremat vishwahiremat merged commit 85072d2 into main Jun 16, 2023
@vishwahiremat vishwahiremat deleted the vishwahiremat/NilPointerFix branch June 16, 2023 21:02
nithyatsu pushed a commit that referenced this pull request Jun 21, 2023
# Description

Added changes to throw an error if conversion fails.

## Issue reference

<!--
We strive to have all PR being opened based on an issue, where the
problem or feature have been discussed prior to implementation.
-->

Fixes: #issue_number

## Checklist

Please make sure you've completed the relevant tasks for this PR, out of
the following list:

* [x] Code compiles correctly
* [ ] Adds necessary unit tests for change
* [ ] Adds necessary E2E tests for change
* [x] Unit tests passing
* [ ] Extended the documentation / Created issue for it

## Auto-generated summary

<!--
GitHub Copilot for docs will auto-generate a summary of the PR
-->

<!--
copilot:all
-->
### <samp>🤖 Generated by Copilot at 495ce78</samp>

### Summary
🐛🔧🚧

<!--
1. 🐛 - This emoji represents a bug fix, since the changes add error
handling to prevent potential bugs or crashes when converting data
models.
2. 🔧 - This emoji represents a tool, since the changes improve the
functionality of the data model conversion functions, which are tools
for working with different versions of data models.
3. 🚧 - This emoji represents a work in progress, since the changes are
part of a larger refactoring effort to support multiple versions of data
models.
-->
This pull request adds error handling to various data model conversion
functions in the `pkg/linkrp/datamodel/converter` package. These
functions are used to transform versioned data models of different
resources to non-versioned ones for the LinkRP service.

> _`DataModelFromVersioned`_
> _Converts with error checks_
> _Fall leaves may wither_

### Walkthrough
* Add error handling to data model conversion functions for various
resources
([link](https://github.com/project-radius/radius/pull/5742/files?diff=unified&w=0#diff-fe1e9091ecc83a1c3582eb567510654663be1b6c5951afe78e0bda54d31fd54bR49-R51),
[link](https://github.com/project-radius/radius/pull/5742/files?diff=unified&w=0#diff-6514b54334e0557b86400723b1b35865b8de5a6772d172722f2efba9894e1e9aR49-R51),
[link](https://github.com/project-radius/radius/pull/5742/files?diff=unified&w=0#diff-9e2a6c473f54c263a21337c76232aa7cdbf203548bcd9bcfe80f31116c95f9a8R53-R55),
[link](https://github.com/project-radius/radius/pull/5742/files?diff=unified&w=0#diff-f724a3f770fd8f4face722e8f6d76e6c7b80dbf83c027b6abd3780424443637dR48-R50),
[link](https://github.com/project-radius/radius/pull/5742/files?diff=unified&w=0#diff-8cceaefe3dd01bf27434e4215db6c298f5d6fe2154db41956253e7d61554e20fR48-R50),
[link](https://github.com/project-radius/radius/pull/5742/files?diff=unified&w=0#diff-0783b94ee282178c7c2442e63467d15599a141a764f75b64c0185aaea968c164R53-R55),
[link](https://github.com/project-radius/radius/pull/5742/files?diff=unified&w=0#diff-8c3eb95b328475de587a4b1cde8ee1d8dc5af838ec3740b6cbe62c25da73baffR49-R51))
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.

3 participants