Skip to content
This repository was archived by the owner on Apr 1, 2026. It is now read-only.

Mount persistent volumes for S2I images#4918

Merged
openshift-merge-robot merged 4 commits into
redhat-developer:mainfrom
feloy:bugfix-4623/wildfly
Sep 10, 2021
Merged

Mount persistent volumes for S2I images#4918
openshift-merge-robot merged 4 commits into
redhat-developer:mainfrom
feloy:bugfix-4623/wildfly

Conversation

@feloy
Copy link
Copy Markdown
Contributor

@feloy feloy commented Jul 16, 2021

What type of PR is this?

/kind bug

What does this PR do / why we need it:

This PR:

  • mounts persistent volumes in /opt/app-root and the deployments directory
  • adds a preStart event to the devfile, to create an initContainer that copies original files in /opt/app-root into the persistent volume

Which issue(s) this PR fixes:

Fixes #4623

PR acceptance criteria:

  • Unit test

  • Integration test

  • Documentation

  • Update changelog

  • I have read the test guidelines

How to test changes / Special notes to the reviewer:

# install the wildfly image stream:

$ oc apply -n openshift -f https://raw.githubusercontent.com/openshift/library/master/arch/x86_64/community/wildfly/imagestreams/wildfly-centos7.json

# get wildfly sources from `tests/examples/source/wildfly/`

$ odo create --s2i wildfly

# check that the devfile contains `events/prestart` section

$ odo push

# check that component is running correctly

or

# get nodejs sources from tests/examples/source/nodejs/

$ odo create --s2i nodejs

# check that the devfile contains `events/prestart` section

$ odo push

# check that component is running correctly

@openshift-ci openshift-ci Bot added the kind/bug Categorizes issue or PR as related to a bug. label Jul 16, 2021
@openshift-ci openshift-ci Bot requested review from dharmit and valaparthvi July 16, 2021 07:12
@feloy feloy changed the title Bugfix 4623/wildfly Mount persistent volumes for S2I images Jul 16, 2021
@feloy feloy changed the title Mount persistent volumes for S2I images [WIP] Mount persistent volumes for S2I images Jul 16, 2021
@openshift-ci openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Jul 16, 2021
@feloy
Copy link
Copy Markdown
Contributor Author

feloy commented Jul 16, 2021

/test v4.7-integration-e2e

@feloy
Copy link
Copy Markdown
Contributor Author

feloy commented Jul 19, 2021

/retest

@feloy
Copy link
Copy Markdown
Contributor Author

feloy commented Jul 19, 2021

/test psi-unit-test-windows

@feloy
Copy link
Copy Markdown
Contributor Author

feloy commented Jul 19, 2021

/test v4.7-integration-e2e

2 similar comments
@feloy
Copy link
Copy Markdown
Contributor Author

feloy commented Jul 20, 2021

/test v4.7-integration-e2e

@feloy
Copy link
Copy Markdown
Contributor Author

feloy commented Jul 20, 2021

/test v4.7-integration-e2e

@feloy feloy mentioned this pull request Jul 21, 2021
8 tasks
@feloy feloy force-pushed the bugfix-4623/wildfly branch from d52066b to 8a2de6a Compare July 22, 2021 18:49
@openshift-ci openshift-ci Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Jul 22, 2021
@feloy feloy force-pushed the bugfix-4623/wildfly branch from 8a2de6a to ff83955 Compare July 22, 2021 19:03
@openshift-ci openshift-ci Bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Jul 22, 2021
@feloy
Copy link
Copy Markdown
Contributor Author

feloy commented Jul 23, 2021

/test psi-kubernetes-integration-e2e

@prietyc123
Copy link
Copy Markdown
Contributor

@feloy I feel there is some real failures with services on kubernetes. I have just overlooked the logs and suspect most of the failures with linking while doing odo push

[ssh:Fedora-32-minikube] [odo] I0723 06:37:05.559229 2095016 pods.go:79] Container Status: {"name":"runtime","state":{"running":{"startedAt":"2021-07-23T06:37:04Z"}},"lastState":{},"ready":true,"restartCount":0,"image":"registry.access.redhat.com/ubi8/nodejs-14:latest","imageID":"docker-pullable://registry.access.redhat.com/ubi8/nodejs-14@sha256:bd40a2c6ad864e848d86ec519c665b6b042b9fa2bd88de6b93946b1055f7ab0b","containerID":"docker://4dcc0c33075505031d05617ab48dea5b63dba1804b0219950537473532e6d82e","started":true}
[ssh:Fedora-32-minikube] [odo] I0723 06:37:05.559254 2095016 pods.go:83] Pod cmp-hxwtvx-app-5b448f6c65-mrn94 is Running
[ssh:Fedora-32-minikube] [odo] I0723 06:37:05.561523 2095016 operators.go:36] Fetching list of operators installed in cluster
[ssh:Fedora-32-minikube] [odo]  ✗  Failed to start component with name "cmp-hxwtvx". Error: Failed to create the component: admission webhook "vservicebinding.kb.io" denied the request: cannot update Service Binding if 'Ready' condition is True. If you want to rebind to another service/application, remove this binding and create a new one.

Could you please have a look.

@feloy feloy force-pushed the bugfix-4623/wildfly branch 2 times, most recently from 706d712 to ff83955 Compare July 27, 2021 07:58
@feloy
Copy link
Copy Markdown
Contributor Author

feloy commented Jul 28, 2021

/test psi-kubernetes-integration-e2e
Retry with SBO v0.9.1

@feloy feloy force-pushed the bugfix-4623/wildfly branch from ff83955 to 142b5e9 Compare August 16, 2021 11:29
@netlify
Copy link
Copy Markdown

netlify Bot commented Aug 16, 2021

✔️ Deploy Preview for odo-docusaurus-preview ready!

🔨 Explore the source changes: ed24f28

🔍 Inspect the deploy log: https://app.netlify.com/sites/odo-docusaurus-preview/deploys/613a0a8e3c92d5000830569d

😎 Browse the preview: https://deploy-preview-4918--odo-docusaurus-preview.netlify.app

@feloy
Copy link
Copy Markdown
Contributor Author

feloy commented Aug 16, 2021

/test psi-unit-test-windows

[ssh:Windows 10] panic: test timed out after 10m0s
[ssh:Windows 10] 
[ssh:Windows 10] goroutine 10 [running]:
[ssh:Windows 10] testing.(*M).startAlarm.func1()
[ssh:Windows 10] 	c:/go/src/testing/testing.go:1628 +0xe6
skipped 142 lines unfold_more
[ssh:Windows 10] github.com/openshift/odo/pkg/watch.WatchAndPush.func1(0xc0005cf3f0, 0xc0005fc1f8, 0xc00036dfd0, 0xc0005e1ec0, 0xc00049f000, 0xc00049efc0, 0xc0005fc200, 0xc00049efe0)
[ssh:Windows 10] 	C:/Users/Admin/amqp_ci_rcv_odo-windows-unit-pr-build_PR_4918/wTSdF/repo/pkg/watch/watch.go:198 +0xf05
[ssh:Windows 10] created by github.com/openshift/odo/pkg/watch.WatchAndPush
[ssh:Windows 10] 	C:/Users/Admin/amqp_ci_rcv_odo-windows-unit-pr-build_PR_4918/wTSdF/repo/pkg/watch/watch.go:187 +0x3da
[ssh:Windows 10] FAIL	github.com/openshift/odo/pkg/watch	600.556s

@feloy
Copy link
Copy Markdown
Contributor Author

feloy commented Aug 16, 2021

/test psi-kubernetes-integration-e2e

 • Failure [611.580 seconds]
 odo devfile push command tests
 /home/fedora/amqp_ci_rcv_odo-minikube-pr-build_PR_4918/KdFfu/repo/tests/integration/devfile/cmd_devfile_push_test.go:18
   verify files are correctly synced
   /home/fedora/amqp_ci_rcv_odo-minikube-pr-build_PR_4918/KdFfu/repo/tests/integration/devfile/cmd_devfile_push_test.go:134
     when project and clonePath is present
     /home/fedora/amqp_ci_rcv_odo-minikube-pr-build_PR_4918/KdFfu/repo/tests/integration/devfile/cmd_devfile_push_test.go:301
       should sync to the correct dir in container [It]
       /home/fedora/amqp_ci_rcv_odo-minikube-pr-build_PR_4918/KdFfu/repo/tests/integration/devfile/cmd_devfile_push_test.go:302
 
       Timed out after 600.000s.
      
       Running odo with args [odo push --v 5]
       Expected process to exit.  It did not.

@feloy
Copy link
Copy Markdown
Contributor Author

feloy commented Aug 16, 2021

/test v4.8-integration-e2e

(passes locally)

• Failure [28.241 seconds]
odo push command tests
/go/src/github.com/openshift/odo/tests/integration/cmd_push_test.go:15
  when push command is executed
  /go/src/github.com/openshift/odo/tests/integration/cmd_push_test.go:76
    should build when a file and a folder is renamed in the directory [It]
    /go/src/github.com/openshift/odo/tests/integration/cmd_push_test.go:114
    No future change is possible.  Bailing out early after 0.539s.
      
    Running odo with args [odo push --context /tmp/592045329]
    Expected
        <int>: 1
    to match exit code:
        <int>: 0

@feloy
Copy link
Copy Markdown
Contributor Author

feloy commented Aug 17, 2021

PVC errors

/test v4.8-integration-e2e

@feloy feloy changed the title [WIP] Mount persistent volumes for S2I images Mount persistent volumes for S2I images Aug 17, 2021
@openshift-ci openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Aug 17, 2021
@valaparthvi
Copy link
Copy Markdown
Contributor

@feloy What was the verdict on this PR? I remember you brought up that this PR might not be relevant with the removal of S2I comp. Are we still bringing this in? Is it RFR?

@feloy
Copy link
Copy Markdown
Contributor Author

feloy commented Aug 27, 2021

@feloy What was the verdict on this PR? I remember you brought up that this PR might not be relevant with the removal of S2I comp. Are we still bringing this in? Is it RFR?

We are waiting for Tomas' opinion if odo create --s2i should also be removed immediately, or later

@dharmit
Copy link
Copy Markdown
Member

dharmit commented Sep 1, 2021

@feloy What was the verdict on this PR? I remember you brought up that this PR might not be relevant with the removal of S2I comp. Are we still bringing this in? Is it RFR?

We are waiting for Tomas' opinion if odo create --s2i should also be removed immediately, or later

@kadel can you PTAL?

@kadel
Copy link
Copy Markdown
Member

kadel commented Sep 2, 2021

We are waiting for Tomas' opinion if odo create --s2i should also be removed immediately, or later

Not in this PR. This PR should be just about fixing the issues with volumes and file permissions

@kadel
Copy link
Copy Markdown
Member

kadel commented Sep 6, 2021

/approve

@openshift-ci openshift-ci Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Sep 6, 2021
@openshift-ci
Copy link
Copy Markdown

openshift-ci Bot commented Sep 6, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kadel

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label Sep 6, 2021
Comment thread pkg/devfile/validate/events.go Outdated
Comment thread pkg/devfile/convert/convert.go
@feloy feloy force-pushed the bugfix-4623/wildfly branch from 4027297 to 6ed4ee4 Compare September 6, 2021 13:27
@openshift-ci openshift-ci Bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Sep 6, 2021
@openshift-ci
Copy link
Copy Markdown

openshift-ci Bot commented Sep 6, 2021

@feloy: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Rerun command
ci/prow/v4.7-integration-e2e ff83955 link /test v4.7-integration-e2e

Full PR test history. Your PR dashboard.

Details

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.

@feloy
Copy link
Copy Markdown
Contributor Author

feloy commented Sep 6, 2021

/test v4.8-integration-e2e

Test passes locally

odo service command tests for OperatorHub
/go/src/github.com/openshift/odo/tests/integration/operatorhub/cmd_service_test.go:18
  Operators are installed in the cluster
  /go/src/github.com/openshift/odo/tests/integration/operatorhub/cmd_service_test.go:31
    a specific operator is installed
    /go/src/github.com/openshift/odo/tests/integration/operatorhub/cmd_service_test.go:129
      when a nodejs component is created
      /go/src/github.com/openshift/odo/tests/integration/operatorhub/cmd_service_test.go:181
        when an Redis instance is created with no name
        /go/src/github.com/openshift/odo/tests/integration/operatorhub/cmd_service_test.go:269
          when odo push is executed
          /go/src/github.com/openshift/odo/tests/integration/operatorhub/cmd_service_test.go:289
            when the service is deleted [BeforeEach]
            /go/src/github.com/openshift/odo/tests/integration/operatorhub/cmd_service_test.go:341
              should delete service definition from devfile.yaml
              /go/src/github.com/openshift/odo/tests/integration/operatorhub/cmd_service_test.go:346
              No future change is possible.  Bailing out early after 13.761s.
      
              Running odo with args [odo push]
              Expected
                  <int>: 1
              to match exit code:
                  <int>: 0

@feloy feloy requested a review from valaparthvi September 8, 2021 16:47
@valaparthvi
Copy link
Copy Markdown
Contributor

@feloy I was unable to push wildfly image to the cluster. Here is the error -

➜  odo push --show-log

Validation
 ✓  Validating the devfile [159314ns]

Creating Services for component wildfly-wildfly-vyyz
 ✓  Services are in sync with the cluster, no changes are required

Creating Kubernetes resources for component wildfly-wildfly-vyyz
 ✓  Waiting for component to start [388ms]
 ✓  Links are in sync with the cluster, no changes are required
 ✓  Waiting for component to start [412ms]

Applying URL changes
 ✓  URL http-8778: http://http-8778-app-myproject.apps.ci-ln-glxhklt-f76d1.origin-ci-int-gce.dev.openshift.com/ created
 ✓  URL http-8080: http://http-8080-app-myproject.apps.ci-ln-glxhklt-f76d1.origin-ci-int-gce.dev.openshift.com/ created

Syncing to component wildfly-wildfly-vyyz
 ✓  Checking file changes for pushing [615818ns]
 ✓  Syncing files to the component [8s]

Executing devfile commands for component wildfly-wildfly-vyyz
 •  Executing s2i-assemble command "/opt/odo/bin/s2i-setup && /opt/odo/bin/assemble-and-restart"  ...
+ set -eo pipefail
+ PV_MNT_PT=/opt/app-root
+ ODO_UTILS_DIR=/opt/odo
+ '[' '!' -f /opt/app-root/conf/supervisor.conf ']'
+ cp -rp /opt/odo/conf /opt/app-root
+ mkdir -p /tmp
+ touch /tmp/.dummy
+ mkdir -p /tmp/src/
+ touch /tmp/src/.dummy
+ [[ /tmp != *\/\s\r\c* ]]
+ mkdir -p /tmp/src
+ touch /tmp/src/.dummy
+ mkdir -p /opt/app-root/src
+ '[' '!' -z true ']'
+ rsync -ar /tmp/projects/ /tmp/src
+ set -eo pipefail
+ export ODO_UTILS_DIR=/opt/odo
+ ODO_UTILS_DIR=/opt/odo
+ export ODO_IMAGE_MAPPINGS_FILE=/opt/odo/language-scripts/image-mappings.json
+ ODO_IMAGE_MAPPINGS_FILE=/opt/odo/language-scripts/image-mappings.json
++ /opt/odo/bin/getlanguage
+ IMAGE_LANG=
+ '[' '!' -z /home/jboss ']'
+ '[' /tmp '!=' /home/jboss ']'
+ '[' -n /opt/app-root/src-backup ']'
+ '[' '!' -d /opt/app-root/src-backup ']'
+ mkdir -p /opt/app-root/src-backup/src
+ rsync -rlO /tmp/src/. /opt/app-root/src-backup/src/
+ b_IFS=' 	
'
+ b_OFS=
+ OIFS=' 	
'
+ IFS='
'
++ ls -A /tmp/src/
+ for file in '`ls -A ${ODO_S2I_SRC_BIN_PATH}/src/`'
+ rm -fr /home/jboss/.dummy
+ for file in '`ls -A ${ODO_S2I_SRC_BIN_PATH}/src/`'
+ rm -fr /home/jboss/.gitignore
+ for file in '`ls -A ${ODO_S2I_SRC_BIN_PATH}/src/`'
+ rm -fr /home/jboss/devfile.yaml
+ for file in '`ls -A ${ODO_S2I_SRC_BIN_PATH}/src/`'
+ rm -fr /home/jboss/pom.xml
+ for file in '`ls -A ${ODO_S2I_SRC_BIN_PATH}/src/`'
+ rm -fr /home/jboss/src
+ OIFS=
+ IFS=' 	
'
+ '[' -n centos7/s2i-base-centos7 ']'
+ '[' centos7/s2i-base-centos7 == redhat-openjdk-18/openjdk18-openshift ']'
+ '[' -f /tmp/src/.s2i/bin/assemble ']'
+ '[' -n '' ']'
+ '[' -n /usr/local/s2i ']'
+ rm -rf /opt/app-root/src/.git
+ /usr/local/s2i/assemble
/opt/odo/bin/assemble-and-restart: line 63: /usr/local/s2i/assemble: No such file or directory
 ✗  Executing s2i-assemble command "/opt/odo/bin/s2i-setup && /opt/odo/bin/assemble-and-restart" [4s]
 ✗  Failed to start component with name "wildfly-wildfly-vyyz". Error: Failed to create the component: command execution failed: unable to execute the run command: unable to exec command [/bin/sh -c /opt/odo/bin/s2i-setup && /opt/odo/bin/assemble-and-restart]: 
+ set -eo pipefail
+ PV_MNT_PT=/opt/app-root
+ ODO_UTILS_DIR=/opt/odo
+ '[' '!' -f /opt/app-root/conf/supervisor.conf ']'
+ cp -rp /opt/odo/conf /opt/app-root
+ mkdir -p /tmp
+ touch /tmp/.dummy
+ mkdir -p /tmp/src/
+ touch /tmp/src/.dummy
+ [[ /tmp != *\/\s\r\c* ]]
+ mkdir -p /tmp/src
+ touch /tmp/src/.dummy
+ mkdir -p /opt/app-root/src
+ '[' '!' -z true ']'
+ rsync -ar /tmp/projects/ /tmp/src
+ set -eo pipefail
+ export ODO_UTILS_DIR=/opt/odo
+ ODO_UTILS_DIR=/opt/odo
+ export ODO_IMAGE_MAPPINGS_FILE=/opt/odo/language-scripts/image-mappings.json
+ ODO_IMAGE_MAPPINGS_FILE=/opt/odo/language-scripts/image-mappings.json
++ /opt/odo/bin/getlanguage
+ IMAGE_LANG=
+ '[' '!' -z /home/jboss ']'
+ '[' /tmp '!=' /home/jboss ']'
+ '[' -n /opt/app-root/src-backup ']'
+ '[' '!' -d /opt/app-root/src-backup ']'
+ mkdir -p /opt/app-root/src-backup/src
+ rsync -rlO /tmp/src/. /opt/app-root/src-backup/src/
+ b_IFS=' 	
'
+ b_OFS=
+ OIFS=' 	
'
+ IFS='
'
++ ls -A /tmp/src/
+ for file in '`ls -A ${ODO_S2I_SRC_BIN_PATH}/src/`'
+ rm -fr /home/jboss/.dummy
+ for file in '`ls -A ${ODO_S2I_SRC_BIN_PATH}/src/`'
+ rm -fr /home/jboss/.gitignore
+ for file in '`ls -A ${ODO_S2I_SRC_BIN_PATH}/src/`'
+ rm -fr /home/jboss/devfile.yaml
+ for file in '`ls -A ${ODO_S2I_SRC_BIN_PATH}/src/`'
+ rm -fr /home/jboss/pom.xml
+ for file in '`ls -A ${ODO_S2I_SRC_BIN_PATH}/src/`'
+ rm -fr /home/jboss/src
+ OIFS=
+ IFS=' 	
'
+ '[' -n centos7/s2i-base-centos7 ']'
+ '[' centos7/s2i-base-centos7 == redhat-openjdk-18/openjdk18-openshift ']'
+ '[' -f /tmp/src/.s2i/bin/assemble ']'
+ '[' -n '' ']'
+ '[' -n /usr/local/s2i ']'
+ rm -rf /opt/app-root/src/.git
+ /usr/local/s2i/assemble
/opt/odo/bin/assemble-and-restart: line 63: /usr/local/s2i/assemble: No such file or directory
: error while streaming command: command terminated with exit code 1

Do you think I am missing something or does this seem unrelated to your changes?

@feloy
Copy link
Copy Markdown
Contributor Author

feloy commented Sep 9, 2021

@feloy I was unable to push wildfly image to the cluster. Here is the error -

➜  odo push --show-log

Validation
 ✓  Validating the devfile [159314ns]

Creating Services for component wildfly-wildfly-vyyz
 ✓  Services are in sync with the cluster, no changes are required

Creating Kubernetes resources for component wildfly-wildfly-vyyz
 ✓  Waiting for component to start [388ms]
 ✓  Links are in sync with the cluster, no changes are required
 ✓  Waiting for component to start [412ms]

Applying URL changes
 ✓  URL http-8778: http://http-8778-app-myproject.apps.ci-ln-glxhklt-f76d1.origin-ci-int-gce.dev.openshift.com/ created
 ✓  URL http-8080: http://http-8080-app-myproject.apps.ci-ln-glxhklt-f76d1.origin-ci-int-gce.dev.openshift.com/ created

Syncing to component wildfly-wildfly-vyyz
 ✓  Checking file changes for pushing [615818ns]
 ✓  Syncing files to the component [8s]

Executing devfile commands for component wildfly-wildfly-vyyz
 •  Executing s2i-assemble command "/opt/odo/bin/s2i-setup && /opt/odo/bin/assemble-and-restart"  ...

[...]

  • /usr/local/s2i/assemble
    /opt/odo/bin/assemble-and-restart: line 63: /usr/local/s2i/assemble: No such file or directory
    : error while streaming command: command terminated with exit code 1

Do you think I am missing something or does this seem unrelated to your changes?

You need to replace /usr/local/s2i in the generated devfile with /usr/libexec/s2i

Not sure if the problem is specific to the wildfly s2i information or if we can fix this in odo

Copy link
Copy Markdown
Contributor

@valaparthvi valaparthvi left a comment

Choose a reason for hiding this comment

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

The code lgtm but do we need 1 or 2 more integration tests to verify these changes or do we already have enough tests? Also, do you think we need to modify unit tests(storage/kubernetes_test.go) to accommodate the new changes?

Command: []string{
"/bin/sh",
"-c",
"[ -d /opt/app-root ] && cp -R /opt/app-root /mnt/ || true",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't understand this script. Does this mean that regardless of any error, it will always return true?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, the script will always return true, even if some error occurs during copying the files. This is to prevent the initContainer to finish as Error, that would terminate the pod immediately

Comment thread pkg/devfile/convert/convert.go Outdated
Comment on lines +337 to +345
var deploymentDir string
for _, env := range s2iEnv {
if env.Name == "ODO_S2I_DEPLOYMENT_DIR" {
deploymentDir = env.Value
break
}
}

separateDeploymentsMount := len(deploymentDir) > 0 && !strings.HasPrefix(deploymentDir, occlient.DefaultAppRootDir)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you move this code to L356 ? I think it will make it easier to understand the code.

@feloy
Copy link
Copy Markdown
Contributor Author

feloy commented Sep 9, 2021

The code lgtm but do we need 1 or 2 more integration tests to verify these changes or do we already have enough tests? Also, do you think we need to modify unit tests(storage/kubernetes_test.go) to accommodate the new changes?

I will work on adding some integration and/or unit tests

@feloy feloy force-pushed the bugfix-4623/wildfly branch from 6ed4ee4 to ed24f28 Compare September 9, 2021 13:22
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Sep 9, 2021

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
10.5% 10.5% Duplication

@feloy feloy requested a review from valaparthvi September 10, 2021 07:06
Copy link
Copy Markdown
Contributor

@valaparthvi valaparthvi left a comment

Choose a reason for hiding this comment

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

Thanks for working on all the requested changes, Philippe.

When("creating a component from s2i wildfly", func() {

BeforeEach(func() {
helper.Cmd("odo", "component", "create", "--s2i", "wildfly", "--project", commonVar.Project).ShouldPass()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we need to add that imagestream or does it work as is?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think the imagestream is installed from this script: scripts/configure-installer-tests-cluster.sh (https://github.com/openshift/odo/blob/main/scripts/configure-installer-tests-cluster.sh#L43)

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Sep 10, 2021
@openshift-merge-robot openshift-merge-robot merged commit 905f212 into redhat-developer:main Sep 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. Required by Prow.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

converted wildfly and dotnet application error out with permission denied

6 participants