-
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
Remove deprecated images and update tests #3587
Remove deprecated images and update tests #3587
Conversation
@amitkrout: The label(s) In 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. |
|
||
// older images which we should remove soon | ||
"rhoar-nodejs/nodejs-8:latest", | ||
"rhoar-nodejs/nodejs-10:latest", |
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.
this seems like a nodejs-10 image, should we remove this too?
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.
Unfortunately i have removed it. Will add it back
"rhoar-nodejs/nodejs-8:latest", | ||
"rhoar-nodejs/nodejs-10:latest", | ||
"bucharestgold/centos7-s2i-nodejs:8.x", | ||
"bucharestgold/centos7-s2i-nodejs:10.x", |
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.
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.
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.
These are all deprecated images
/approve |
[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 |
aea86ab
to
0989465
Compare
@@ -315,6 +314,7 @@ func MockImageStream() *imagev1.ImageStream { | |||
"12": "docker.io/rhscl/nodejs-12-rhel7:latest", | |||
"10": "docker.io/rhscl/nodejs-10-rhel7:latest", | |||
"8": "docker.io/rhoar-nodejs/nodejs-8:latest", | |||
|
|||
// an unspported one |
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.
AFAIU "8": and "6": both are unsupported one then why not shift this comment one step up
Everything looks good to me 👍 /lgtm |
/retest Please review the full test history for this PR and help us cut down flakes. |
We have a genuine travis failure https://travis-ci.com/github/openshift/odo/jobs/363165202#L574 |
Codecov Report
@@ Coverage Diff @@
## master #3587 +/- ##
==========================================
- Coverage 45.73% 45.69% -0.04%
==========================================
Files 122 122
Lines 12237 12230 -7
==========================================
- Hits 5596 5589 -7
Misses 6090 6090
Partials 551 551
Continue to review full report at Codecov.
|
1a7a256
to
306ec7d
Compare
fefe5ad
to
1c30cbc
Compare
b08528c
to
4a331d2
Compare
// an unspported one | ||
"8": "docker.io/rhoar-nodejs/nodejs-8:latest", | ||
"6": "docker.io/rhoar-nodejs/nodejs-6:latest", |
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.
Why not remove nodejs-6 image as well?
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.
Kept it here just for UTs validation step on more values.
Validation done here - https://github.com/openshift/odo/pull/3587/files#diff-1dfbb1796e8d3ca31e26fe9d003bb488R168
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.
oh. my bad
@@ -583,6 +583,12 @@ func componentTests(args ...string) { | |||
output := helper.CmdShouldPass("odo", append(args, "create", "nodejs:latest", componentName, "--app", appName, "--project", project, "--context", context)...) | |||
Expect(output).NotTo(ContainSubstring("Warning")) | |||
}) | |||
|
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.
if there is java specific code involved as well, you should change the PR description. I see the supported-java.json file being added as well. will be a good practice.
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.
Make sense 👍
I could have split it into two prs but currently it is well organized so i don't want to split again. Yes, i will update the pr title.
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.
Done
/hold cancel |
* Remove deprecated nodejs 8 images * Update README * Updated README * Addressed review comments * Fixing unit test failure * Addressed review comment * Fixing travis failure * Checking oc version * Applying supported image imagestream * Applying supported image imagestream * Added supported is * Debugging * Cleanup * Updated test doc * Cleanup
What type of PR is this?
/kind cleanup
/kind deprecation
/kind documentation
What does does this PR do / why we need it:
Clean up
Which issue(s) this PR fixes:
Fixes need for redhat-developer/odo-init-image#64
PR acceptance criteria:
Unit test
Integration test : NA - Tests are running on all s2i images in e2e scenario.
Documentation
How to test changes / Special notes to the reviewer:
old unsupported images are removed