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

Adds ability to pick version when deploying #565

Merged
merged 1 commit into from
Aug 3, 2018
Merged

Adds ability to pick version when deploying #565

merged 1 commit into from
Aug 3, 2018

Conversation

cdrage
Copy link
Member

@cdrage cdrage commented Jul 5, 2018

This adds the ability to pick both the image as well as the image
version when deploying, for example, using odo create nodejs:8 foobar

@cdrage
Copy link
Member Author

cdrage commented Jul 5, 2018

This relies on #558 being merged in, after this is merged in, I will add unit tests / continue working on this.

This PR adds the option of specifying an image.

To test, use: odo create nodejs:latest foobar or odo create nodejs:VERSION.

TODO:

  • Add tests

@cdrage cdrage force-pushed the pick-what-version-create branch from de031b6 to 16a99f4 Compare July 5, 2018 20:03
@cdrage
Copy link
Member Author

cdrage commented Jul 9, 2018

This relies on #558 and I cannot add tests until #558 is reviewed (as the underlying structure may change).

Just need reviewing 👍 @surajnarwade @ashetty1 @kadel #558

@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 9, 2018
@surajnarwade
Copy link
Contributor

In this case,

 odo create nodejs:6
nodejs:6 is not a valid name:  a DNS-1123 label must consist of lower case alphanumeric characters or '-', and must start and end with an alphanumeric character (e.g. 'my-name',  or '123-abc', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?')

user have to specify name as well

@surajnarwade
Copy link
Contributor

We should also update examples in odo create -h to show this feature and it's limitation.
other wise, this looks awesome to me :)
@cdrage

@cdrage cdrage mentioned this pull request Jul 16, 2018
Closed
@jorgemoralespou
Copy link
Contributor

@cdrage I'm assuming that:

  • Component name, Service, deployment, etc... names are based on image name rather than full name, as @surajnarwade was having an error. (e.g. when using nodejs:6, the service and deployment and component name are nodejs)
  • If there's a latest, one can just omit that tag (e.g. nodejs:latest is similar to nodejs)

@cdrage
Copy link
Member Author

cdrage commented Jul 17, 2018

@jorgemoralespou

Yup, that is both correct. The bug that @surajnarwade encountered was me forgetting to default to the componentName (rather than the version).

Both of your points are true regarding the versioning as well as latest being the default if the tag is omitted.

@kadel kadel added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Jul 25, 2018
@cdrage cdrage force-pushed the pick-what-version-create branch 3 times, most recently from 7aefffd to 71379d0 Compare July 30, 2018 18:41
@cdrage
Copy link
Member Author

cdrage commented Jul 30, 2018

@kadel @surajnarwade @syamgk @mik-dass @ashetty1

With the exception of unit tests being written (WIP), this is ready for review. Feel free to test it out!

@cdrage cdrage force-pushed the pick-what-version-create branch from 71379d0 to 1dc051e Compare July 30, 2018 19:05
ashetty1
ashetty1 approved these changes Jul 31, 2018
View reviewed changes
Copy link
Contributor

@ashetty1 ashetty1 left a comment

Choose a reason for hiding this comment

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

Tested this locally. LGTM

@kadel kadel removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Jul 31, 2018
@cdrage cdrage removed the status/blocked Denotes an issue or PR that is blocked on something (e.g., issue/PR in different repo) label Jul 31, 2018
@cdrage cdrage force-pushed the pick-what-version-create branch 2 times, most recently from 9161916 to 0f7b1b1 Compare August 1, 2018 18:26
@cdrage
Copy link
Member Author

cdrage commented Aug 1, 2018

Tests written, please review: @kadel @surajnarwade @ashetty1 (since more unit tests were added) @syamgk

@cdrage cdrage mentioned this pull request Aug 1, 2018
Merged
@syamgk
Copy link
Member

syamgk commented Aug 2, 2018

Tested with minishift and working for me
so LGTM

kadel
kadel reviewed Aug 2, 2018
View reviewed changes
pkg/catalog/catalog.go Outdated
@@ -61,6 +61,32 @@ func Exists(client *occlient.Client, componentType string) (bool, error) {
return false, nil
}

// VersionExists checks if that version exists, returns true if the given version exists, false if not
// TODO: Write a test
Copy link
Member

Choose a reason for hiding this comment

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

I guess this can be removed now ;-)

pkg/component/labels/labels.go Outdated
@@ -10,6 +10,9 @@ const ComponentLabel = "app.kubernetes.io/component-name"
// ComponentTypeLabel is kubernetes that identifies type of a component
const ComponentTypeLabel = "app.kubernetes.io/component-type"

// ComponentTypeLabel is kubernetes that identifies type of a component
Copy link
Member

Choose a reason for hiding this comment

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

copy/paste?. should be ComponentTypeVersion
Plus it looks like that comment doesn't make sense.

pkg/occlient/occlient_test.go Outdated
@@ -465,7 +465,7 @@ func TestAddLabelsToArgs(t *testing.T) {
}
}

func Test_parseImageName(t *testing.T) {
func Test_ParseImageName(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

should be TestParseImageName

@cdrage cdrage force-pushed the pick-what-version-create branch from 0f7b1b1 to 883a3ae Compare August 2, 2018 17:24
@cdrage
Adds ability to pick version when deploying
38432c9 
This adds the ability to pick both the image as well as the image
version when deploying, for example, using `odo create nodejs:8 foobar`
@cdrage cdrage force-pushed the pick-what-version-create branch from 883a3ae to 38432c9 Compare August 2, 2018 17:39
@cdrage
Copy link
Member Author

cdrage commented Aug 2, 2018

@kadel Updated! ready for another round of testing 👍

@kadel
Copy link
Member

kadel commented Aug 3, 2018

Next time, please just add new commits, instead of squashing all changes into one. It will make reviews much easier.

@kadel kadel merged commit 1517608 into redhat-developer:master Aug 3, 2018
@cdrage cdrage deleted the pick-what-version-create 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 approved these changes

@anmolbabu anmolbabu Awaiting requested review from anmolbabu

@surajnarwade surajnarwade Awaiting requested review from surajnarwade

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 @jorgemoralespou @syamgk @kadel @ashetty1