Adds support for container-overrides
#155
Conversation
Signed-off-by: Dharmit Shah <shahdharmit@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dharmit The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
|
||
| // containerOverridesHandler modifies the container component to include any container-overrides values in the devfile. | ||
| // It does so by merging the devfile JSON with the JSON string representation of container-overrides field | ||
| func containerOverridesHandler(comp v1.Component, container *corev1.Container) (*corev1.Container, error) { |
There was a problem hiding this comment.
I find it a bad idea to pass a pointer and return a pointer. But I'm struggling to do otherwise because of the way runtime.DefaultUnstructuredConverter.ToUnstructured works.
I'm open to ideas/suggestions.
go.mod
Outdated
|
|
||
| require ( | ||
| github.com/devfile/api/v2 v2.2.0 | ||
| github.com/devfile/library v1.2.1-0.20220308191614-f0f7e11b17de |
There was a problem hiding this comment.
I think you may need to go mod tidy
Signed-off-by: Dharmit Shah <shahdharmit@gmail.com>
Codecov ReportBase: 58.95% // Head: 58.75% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #155 +/- ##
==========================================
- Coverage 58.95% 58.75% -0.20%
==========================================
Files 36 36
Lines 4037 4083 +46
==========================================
+ Hits 2380 2399 +19
- Misses 1518 1535 +17
- Partials 139 149 +10
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
Signed-off-by: Dharmit Shah <shahdharmit@gmail.com>
| if *errHandler != nil && !errors.Is(*errHandler, &attributes.KeyNotFoundError{Key: key}) { | ||
| return nil, *errHandler | ||
| } | ||
| // no need to continue any further if container-overrides wasn't used for the container component | ||
| if val == nil { | ||
| return nil, nil | ||
| } |
There was a problem hiding this comment.
| if *errHandler != nil && !errors.Is(*errHandler, &attributes.KeyNotFoundError{Key: key}) { | |
| return nil, *errHandler | |
| } | |
| // no need to continue any further if container-overrides wasn't used for the container component | |
| if val == nil { | |
| return nil, nil | |
| } | |
| if *errHandler != nil { | |
| if errors.Is(*errHandler, &attributes.KeyNotFoundError{Key: key}){ | |
| // no need to continue any further if container-overrides wasn't used for the container component | |
| return nil, nil | |
| } | |
| return nil, *errHandler | |
| } | |
| func convertUnstructuredToResource(u unstructured.Unstructured, obj interface{}) error { | ||
| return runtime.DefaultUnstructuredConverter.FromUnstructured(u.UnstructuredContent(), obj) | ||
| } |
There was a problem hiding this comment.
nit:
To keep this function consistent with convertResourceToUnstructured, I think the receiving argument for this function should be the same as return value of the other one.
| func convertUnstructuredToResource(u unstructured.Unstructured, obj interface{}) error { | |
| return runtime.DefaultUnstructuredConverter.FromUnstructured(u.UnstructuredContent(), obj) | |
| } | |
| func convertUnstructuredToResource(content map[string]interface{}, obj interface{}) error { | |
| return runtime.DefaultUnstructuredConverter.FromUnstructured(content, obj) | |
| } |
|
@valaparthvi: changing LGTM is restricted to collaborators DetailsIn response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
Can we close this in favor of #157? |
|
@valaparthvi closing this as you requested so. |
3 participants
Signed-off-by: Dharmit Shah shahdharmit@gmail.com
What does this PR do?:
Adds support for
container-overridesattribute, as described in devfile/api#920 (comment).Which issue(s) this PR fixes:
Fixes part of #936
PR acceptance criteria:
Testing and documentation do not need to be complete in order for this PR to be approved. We just need to ensure tracking issues are opened.
Unit/Functional tests
QE Integration test
Documentation
Client Impact
Gosec scans
How to test changes / Special notes to the reviewer: