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.

Sign up for GitHub

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

Jump to bottom

Show tags / versions when doing odo catalog list #558

Merged
merged 1 commit into from
Jul 27, 2018
Merged

Show tags / versions when doing odo catalog list #558

merged 1 commit into from
Jul 27, 2018

Conversation

cdrage
Copy link
Member

@cdrage cdrage commented Jul 3, 2018

When using odo catalog list the tags will now be listed:

github.com/redhat-developer/odo  add-versioning ✗                                                                                                                                                                                                                                                                                                                    7d ⚑  ⍉
▶ ./odo catalog list
NAME        TAGS
dotnet      2.0,latest
httpd       2.4,latest
nginx       1.10,1.12,1.8,latest
nodejs      0.10,4,6,8,latest
perl        5.16,5.20,5.24,latest
php         5.5,5.6,7.0,7.1,latest
python      2.7,3.3,3.4,3.5,3.6,latest
ruby        2.0,2.2,2.3,2.4,latest
wildfly     10.0,10.1,8.1,9.0,latest

@cdrage
Copy link
Member Author

cdrage commented Jul 3, 2018

This will probably conflict with your PR @surajnarwade since we're modifying the same sections 👍

@surajnarwade
Copy link
Contributor

yeah @cdrage , this PR looks good to me though :)

@cdrage cdrage mentioned this pull request Jul 5, 2018
Merged
@cdrage
Copy link
Member Author

cdrage commented Jul 16, 2018

Ping @kadel @ashetty1 :)

@cdrage cdrage mentioned this pull request Jul 16, 2018
Closed
surajnarwade
surajnarwade approved these changes Jul 17, 2018
View reviewed changes
Copy link
Contributor

@surajnarwade surajnarwade left a comment

Choose a reason for hiding this comment

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

Looks good to me and WFM as well

@surajnarwade
Copy link
Contributor

@cdrage , now we dont have conflicts between this and service catalog PR since both are two different commands now

@kadel
Copy link
Member

kadel commented Jul 17, 2018

Looks good, but there are no tests :-(

@cdrage
Copy link
Member Author

cdrage commented Jul 18, 2018

Hey @kadel so I started on writing tests and ironically @ashetty1 had already written some! His PR is here: #478

After #478 is merged, I'll be able to add a few tests to cover the listing of tags. I've already began rebasing this PR against #478

@cdrage cdrage added the status/blocked Denotes an issue or PR that is blocked on something (e.g., issue/PR in different repo) label Jul 19, 2018
@surajnarwade
Copy link
Contributor

@cdrage , Since #478 is merged now ? can you please rebase and send so that we can merge this :)

@surajnarwade surajnarwade removed the status/blocked Denotes an issue or PR that is blocked on something (e.g., issue/PR in different repo) label Jul 25, 2018
@cdrage cdrage force-pushed the add-versioning branch 2 times, most recently from 59a8aad to 5dc99d1 Compare July 25, 2018 17:16
@cdrage
Copy link
Member Author

cdrage commented Jul 25, 2018

Ready for review! @surajnarwade @kadel @ashetty1 @mik-dass @syamgk

@surajnarwade
Copy link
Contributor

Tests looks good to me

ashetty1
ashetty1 reviewed Jul 26, 2018
View reviewed changes
pkg/catalog/catalog_test.go Outdated

// Function taken from occlient_test.go
// fakeImageStream gets imagestream for the reactor
func fakeImageStream(imageName string, namespace string, tags []string) *imagev1.ImageStream {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should move these helper functions to a different file under occlient package and just call them from there. WDYT? We could take up that exercise post PR merge too.

Cc: @kadel

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I was trying to move them to fakeclient.go or making them public within occlient_test.go (which didn't work).

To be honest, I had to heavily modify these functions to work specifically for catalog list (since I'm passing in tags []string and modifying it. I would be more keen towards leaving them in the catalog_test.go file.

Copy link
Member

Choose a reason for hiding this comment

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

Those helper function should be moved to something like testingutil pkg and merged into just one function, so we can share it between different tests.
Ideally in this PR 😬 If not then we should at least open new Issue so we don't forget to clean this.

Copy link
Member Author

Choose a reason for hiding this comment

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

@kadel No worries, I'll move them to a testingutil pkg :)

@cdrage
Show tags / versions when doing odo catalog list
c58cf9a 
When using `odo catalog list` the tags will now be listed:

```
github.com/redhat-developer/odo  add-versioning ✗                                                                                                                                                                                                                                                                                                                    7d ⚑  ⍉
▶ ./odo catalog list
NAME        TAGS
dotnet      2.0,latest
httpd       2.4,latest
nginx       1.10,1.12,1.8,latest
nodejs      0.10,4,6,8,latest
perl        5.16,5.20,5.24,latest
php         5.5,5.6,7.0,7.1,latest
python      2.7,3.3,3.4,3.5,3.6,latest
ruby        2.0,2.2,2.3,2.4,latest
wildfly     10.0,10.1,8.1,9.0,latest

```
@cdrage
Copy link
Member Author

cdrage commented Jul 26, 2018

@kadel @surajnarwade @syamgk @ashetty1 @syamgk

Tests have been moved to a different package. Ready for review 👍

mik-dass
mik-dass approved these changes Jul 27, 2018
View reviewed changes
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.

LGTM :)

@syamgk
Copy link
Member

syamgk commented Jul 27, 2018

LGTM

syamgk
syamgk reviewed Jul 27, 2018
View reviewed changes
pkg/testingutil/imagestreams.go
@@ -0,0 +1,43 @@
package testingutil
Copy link
Member

Choose a reason for hiding this comment

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

wow! this way of separating function looks nice!

@cdrage cdrage merged commit 9304b86 into redhat-developer:master Jul 27, 2018
@cdrage cdrage deleted the add-versioning branch August 23, 2018 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kadel kadel kadel left review comments

@syamgk syamgk syamgk approved these changes

@ashetty1 ashetty1 ashetty1 left review comments

@mik-dass mik-dass mik-dass approved these changes

@surajnarwade surajnarwade surajnarwade approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@cdrage @surajnarwade @kadel @syamgk @mik-dass @ashetty1