-
Notifications
You must be signed in to change notification settings - Fork 244
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
Ignore Configmap mounted in DeploymentConfig #4193
Ignore Configmap mounted in DeploymentConfig #4193
Conversation
Hi @mcouliba. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
/ok-to-test |
@@ -147,6 +147,11 @@ func List(client *occlient.Client, componentName string, applicationName string) | |||
continue | |||
} | |||
|
|||
// We should ignore ConfigMap (while PR2142 and PR2601 are not fixed) | |||
if client.IsVolumeAnConfigMap(volumeMount.Name, dc) { |
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.
The client currently has no such function. Thus the build is failing
pkg/storage/storage.go:151:12: client.IsVolumeAnConfigMap undefined (type *occlient.Client has no field or method IsVolumeAnConfigMap)
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.
Yes sorry! Just committed it
Can you please provide some steps to test these changes? |
To reproduce the error, please follow:
Without the fix, you will have the message 'no PVC associated with Volume Mount volume-xxxx' |
@mik-dass The step "make test-cmd-push" failed but where can I get more logs about the error. |
Codecov Report
@@ Coverage Diff @@
## master #4193 +/- ##
==========================================
- Coverage 42.28% 42.21% -0.07%
==========================================
Files 154 155 +1
Lines 13074 13210 +136
==========================================
+ Hits 5528 5577 +49
- Misses 6954 7025 +71
- Partials 592 608 +16
Continue to review full report at Codecov.
|
The odo verbose error log is here https://travis-ci.com/github/openshift/odo/jobs/428010783#L1147 |
/retest |
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
hello @mik-dass, I have spent some time trying to fix the Travis CI. It is very weird because each time I run a test ( Is it a expected behaviour? If so, how can I fix my PR to make it all green? |
The tests are failing due a flake #3454. You can restart the jobs to make Travis go green. |
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.
Don't have a strong opinion on the question/suggestion I made so approving.
/approve
for _, volume := range dc.Spec.Template.Spec.Volumes { | ||
if volume.Name == volumeMountName { | ||
if volume.ConfigMap != nil { | ||
return 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.
Wouldn't if volume.Name == volumeMountName && volume.ConfigMap != nil
be better? My only reasoning for saying it "better" is that it is a one-liner and reduces a nested if
. :)
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dharmit 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 |
What type of PR is this?
/kind bug
What does does this PR do / why we need it:
When creating applications on OpenShift Kubernetes, a (12-factor) best practice is to externalize the configuration in a ConfigMap. When mounting this ConfigMap in the DC, odo is not working anymore because it expects a volume with a PVC. As odo cannot manage ConfigMap yet, we should ignore them (as we ignore empty volume) to apply best practices while using odo.
Which issue(s) this PR fixes:
This is an alternance for waiting for the PR2142 and PR2601
PR acceptance criteria:
Unit test
Integration test
Documentation
I have read the test guidelines
How to test changes / Special notes to the reviewer: