-
Notifications
You must be signed in to change notification settings - Fork 243
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
Adds storage Push() for devfile components. #4407
Adds storage Push() for devfile components. #4407
Conversation
Skipping CI for Draft Pull Request. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mik-dass The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
cb276be
to
95fc23f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Just glanced through the PR to see how it may affect devfile/library.
- A few thoughts from odo's perspective if you will 😉
continue | ||
} | ||
volumeNameToPVCName[pvc.Labels[storagelabels.DevfileStorageLabel]] = pvc.Name | ||
} | ||
|
||
// Get PVC volumes and Volume Mounts | ||
containers, pvcVolumes, err := storage.GetPVCAndVolumeMount(containers, volumeNameToPVCName, containerNameToVolumes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so i was looking to extract this logic out, so that the library can return the updated containers with volume mounts and the pvc volumes after reading the devfile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have only modified the network calls in this function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, after this merged, I can look to consolidate this in devfile/library call; so anyone calling devfile/library generator func like generator.GetVolume()
will return the pvcVolumes
and updated container volumeMounts 👍
d958305
to
97007be
Compare
/retest |
95905d8
to
52ed1f6
Compare
continue | ||
} | ||
volumeNameToPVCName[pvc.Labels[storagelabels.DevfileStorageLabel]] = pvc.Name | ||
} | ||
|
||
// Get PVC volumes and Volume Mounts | ||
containers, pvcVolumes, err := storage.GetPVCAndVolumeMount(containers, volumeNameToPVCName, containerNameToVolumes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, after this merged, I can look to consolidate this in devfile/library call; so anyone calling devfile/library generator func like generator.GetVolume()
will return the pvcVolumes
and updated container volumeMounts 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
for _, pvc := range pvcs { | ||
found := false | ||
for _, volumeMount := range volumeMounts { | ||
if volumeMount.Name == pvc.Name+"-vol" { | ||
// this volume mount is used by a PVC | ||
validVolumeMounts[volumeMount.Name] = true | ||
|
||
found = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont think you need this check at all, if you think about it; you can have a volume component in a devfile(and a pvc for it) and not be mounted on any container component. Is it really an err case?
I think this goes back to the case where odo should only create pvc for volume components that are used in container components but i guess it can be a separate issue on it its own
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont think you need this check at all, if you think about it; you can have a volume component in a devfile(and a pvc for it) and not be mounted on any container component. Is it really an err case?
Yes, we decided not to support such a case as this may lead to unintentional PVC usage.
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
2 similar comments
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, good catch on source volume
/retest |
/retest |
It also adds CLI messages for creation and deletion of storage. Signed-off-by: mik-dass <mrinald7@gmail.com>
… the kubernetes storage client
…nterface and adds some comments.
…ontainers and the source volume
rebased |
@mik-dass: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
What type of PR is this?
/kind code-refactoring
What does does this PR do / why we need it:
It also adds CLI messages for creation and deletion of storage.
Which issue(s) this PR fixes:
Fixes part of #4298
PR acceptance criteria:
Unit test
Integration test
Documentation
I have read the test guidelines
How to test changes / Special notes to the reviewer:
odo push
.