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

add owner reference to ingress and tlssecert metadata #2841

Conversation

yangcao77
Copy link
Contributor

What type of PR is this?

Uncomment only one /kind line, and delete the rest.
For example, > /kind bug would simply become: /kind bug

/kind bug

What does does this PR do / why we need it:
add owner reference to ingress and tlssecert metadata

Which issue(s) this PR fixes:
Fixes #2808

How to test changes / Special notes to the reviewer:

  1. get the sample nodejs devfile from: https://github.com/elsony/devfile-registry/tree/master/devfiles/nodejs
  2. odo create nodejs
  3. odo url create testurl1 --host 1.2.3.4.com --secure
  4. odo push
    => Verify that creates a deployment nodejs, a ingress testurl1, and a tlssecret nodejs-tlssecret
  5. kubectl delete deploy nodejs
    => verify the ingress and the tlssecret have been deleted along with the deployment

@openshift-ci-robot openshift-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Apr 7, 2020
@openshift-ci-robot
Copy link
Collaborator

Hi @yangcao77. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

@openshift-ci-robot openshift-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. Required by Prow. label Apr 7, 2020
@girishramnani
Copy link
Contributor

/ok-to-test

@openshift-ci-robot openshift-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. Required by Prow. labels Apr 8, 2020
Copy link
Member

@dharmit dharmit left a comment

Choose a reason for hiding this comment

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

Works as expected when I do odo delete. Code looks good as well.

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Apr 9, 2020
@dharmit
Copy link
Member

dharmit commented Apr 9, 2020

Seeing following message on both 4.2 & 4.3 CI failures (4.2 link & 4.3 link):

 odo preference and config command tests
/go/src/github.com/openshift/odo/tests/integration/cmd_pref_config_test.go:20
  when using --now with config command
  /go/src/github.com/openshift/odo/tests/integration/cmd_pref_config_test.go:342
    should successfully set and unset variables [It]
[Fail] odo preference and config command tests when using --now with config command [It] should successfully set and unset variables 
/go/src/github.com/openshift/odo/tests/helper/helper_run.go:34 

Looks like it's hitting #2642

/test v4.2-integration-e2e-benchmark
/test v4.3-integration-e2e-benchmark

@amitkrout
Copy link
Contributor

[odo] 
 ✓  Syncing files to the component [854ms]
[odo]  •  Building component  ...
[odo] 
 ✓  Building component [3s]
[odo]  ✓  Changes successfully pushed to component
Checking http://nodejs-8080-app-kcnucotirq.apps.ci-op-c11wfsbd-8cf2b.origin-ci-int-aws.dev.rhcloud.com, for UPDATED!
try 0 of 30
Deleting project: kcnucotirq
Running odo with args [odo project delete kcnucotirq -f]
[odo]  ✗  The project kcnucotirq does not exist. Please check the list of projects using `odo project list`
• Failure in Spec Teardown (AfterEach) [48.281 seconds]
odo push command tests
/go/src/github.com/openshift/odo/tests/integration/cmd_push_test.go:16
  when push command is executed [AfterEach]
  /go/src/github.com/openshift/odo/tests/integration/cmd_push_test.go:112
    should not build when no changes are detected in the directory and build when a file change is detected
    /go/src/github.com/openshift/odo/tests/integration/cmd_push_test.go:113
    No future change is possible.  Bailing out early after 8.202s.
      
    Running odo with args [odo project delete kcnucotirq -f]
    Expected
        <int>: 1
    to match exit code:
        <int>: 0
    /go/src/github.com/openshift/odo/tests/helper/helper_run.go:34

Details - https://prow.svc.ci.openshift.org/view/gcs/origin-ci-test/pr-logs/pull/openshift_odo/2841/pull-ci-openshift-odo-master-v4.3-integration-e2e-benchmark/1820#1:build-log.txt%3A484
May be a new flake has been introduced
/test v4.3-integration-e2e-benchmark

@codecov
Copy link

codecov bot commented Apr 9, 2020

Codecov Report

Merging #2841 into master will decrease coverage by 0.13%.
The diff coverage is 26.08%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2841      +/-   ##
==========================================
- Coverage   43.23%   43.09%   -0.14%     
==========================================
  Files          97       97              
  Lines        8878     8888      +10     
==========================================
- Hits         3838     3830       -8     
- Misses       4673     4690      +17     
- Partials      367      368       +1     
Impacted Files Coverage Δ
...g/devfile/adapters/kubernetes/component/adapter.go 27.61% <0.00%> (-0.27%) ⬇️
pkg/url/url.go 26.66% <0.00%> (-1.41%) ⬇️
pkg/kclient/secrets.go 85.71% <85.71%> (-3.76%) ⬇️
pkg/watch/watch.go 63.52% <0.00%> (-1.77%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f07025a...a893823. Read the comment docs.

@mik-dass
Copy link
Contributor

mik-dass commented Apr 13, 2020

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. Required by Prow. label Apr 13, 2020
Signed-off-by: Stephanie <stephanie.cao@ibm.com>
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Apr 13, 2020
@yangcao77
Copy link
Contributor Author

/retest

@yangcao77
Copy link
Contributor Author

/test v4.2-integration-e2e-benchmark

@yangcao77
Copy link
Contributor Author

/unhold

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. Required by Prow. label Apr 13, 2020
@yangcao77
Copy link
Contributor Author

@mik-dass The delete test has been updated. Can you help add the approve and lgtm label?

Copy link
Contributor

@mik-dass mik-dass left a comment

Choose a reason for hiding this comment

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

works for me locally and CI is green.

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Apr 14, 2020
@girishramnani
Copy link
Contributor

/approve

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: girishramnani

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label Apr 14, 2020
@mik-dass
Copy link
Contributor

mik-dass commented Apr 14, 2020

@mik-dass The delete test has been updated. Can you help add the approve and lgtm label?

@yangcao77 I have added the lgtm label. I don't have the approval rights though.

@openshift-merge-robot openshift-merge-robot merged commit 78a1312 into redhat-developer:master Apr 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. ok-to-test Indicates a non-member PR verified by an org member that is safe to test.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing owner reference in ingress created for devfile component
7 participants