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

add parameter map population in non-interactive service creation #2040

Merged
merged 5 commits into from
Sep 4, 2019
Merged

add parameter map population in non-interactive service creation #2040

merged 5 commits into from
Sep 4, 2019

Conversation

girishramnani
Copy link
Contributor

What is the purpose of this change? What does it change?

add parameter map population in non-interactive service creation.

Was the change discussed in an issue?

fixes #2033

How to test changes?

confirm that env variables get populated and show correctly

$ odo service create dh-postgresql-apb dh-postgresql-apb --plan dev -p postgresql_database=mydata -p postgresql_password=secret -p postgresql_user=luke -p postgresql_version=10

$ oc describe pod/<postgres-pod> | grep -A3 Env
    Environment:
      POSTGRESQL_PASSWORD:  secret
      POSTGRESQL_USER:      luke
      POSTGRESQL_DATABASE:  mydata

dharmit
dharmit suggested changes Aug 23, 2019
View reviewed changes
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.

Code looks good and works well upon testing on my setup. Need some changes in test, IMO.

tests/integration/servicecatalog/service_test.go Outdated Show resolved Hide resolved
tests/integration/servicecatalog/service_test.go Outdated Show resolved Hide resolved
@girishramnani
Copy link
Contributor Author

test is failing, trying to find the best way to get Environment output without fetching the pod name

@girishramnani
Copy link
Contributor Author

/retest

@girishramnani
Copy link
Contributor Author

/retest

mohammedzee1000
mohammedzee1000 approved these changes Aug 27, 2019
View reviewed changes
Copy link
Contributor

@mohammedzee1000 mohammedzee1000 left a comment

Choose a reason for hiding this comment

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

looking good
/approve

@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 Aug 27, 2019
cdrage
cdrage reviewed Aug 27, 2019
View reviewed changes
Copy link
Member

@cdrage cdrage left a comment

Choose a reason for hiding this comment

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

Shouldn't we also add support for comma-separating the parameters?

Ex. -p postgresql_database=mydata,postgresql_user=foobar

pkg/odo/cli/service/create.go
@@ -127,6 +127,16 @@ func (o *ServiceCreateOptions) Complete(name string, cmd *cobra.Command, args []
if len(args) == 2 {
o.ServiceName = args[1]
}

o.ParametersMap = make(map[string]string)
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick, but could you add a comment above this line explaining what all of this does? Even if it's a line saying about using odo service create dh-postgresql-apb dh-postgresql-apb --plan dev -p postgresql_database=mydata -p postgresql_password=secret -p postgresql_user=luke -p postgresql_version=10

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved

tests/integration/servicecatalog/service_test.go

It("should be able to create postgresql with env", func() {
helper.CmdShouldPass("odo", "service", "create", "dh-postgresql-apb", "--project", project, "--app", app,
"--plan", "dev", "-p", "postgresql_user=lukecage", "-p", "postgresql_password=secret",
Copy link
Member

Choose a reason for hiding this comment

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

Could we add tests regarding using postgresql_user= or when someone enters it multiple times?

Ex. odo service create dh-postgresql-apb dh-postgresql-apb --plan dev -p postgresql_database=mydata -p postgresql_database=foobar

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved

@girishramnani
Copy link
Contributor Author

Shouldn't we also add support for comma-separating the parameters?

Ex. -p postgresql_database=mydata,postgresql_user=foobar

I think that is handled by cobra automatically

@girishramnani
Copy link
Contributor Author

/retest

cdrage
cdrage approved these changes Sep 3, 2019
View reviewed changes
Copy link
Member

@cdrage cdrage 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 the changes, this LGTM!

/approve

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cdrage, mohammedzee1000

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

@girishramnani
Copy link
Contributor Author

girishramnani commented Sep 4, 2019 •
edited 

[odo]  ✗  Unable to connect to OpenShift cluster, is it down?


�[91m�[1m• Failure in Spec Teardown (AfterEach) [1.761 seconds]�[0m
odo storage command
�[90m/go/src/github.com/openshift/odo/tests/integration/cmd_storage_test.go:13�[0m
  �[91m�[1mwhen running help for storage command [AfterEach]�[0m
  �[90m/go/src/github.com/openshift/odo/tests/integration/cmd_storage_test.go:35�[0m
    should display the help
    �[90m/go/src/github.com/openshift/odo/tests/integration/cmd_storage_test.go:36�[0m

/retest

@girishramnani
Copy link
Contributor Author

LGTMing as I see 3 approvals
/lgtm

@openshift-ci-robot
Copy link
Collaborator

@girishramnani: you cannot LGTM your own PR.

In response to this:

LGTMing as I see 3 approvals
/lgtm

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.

@dharmit
Copy link
Member

dharmit commented Sep 4, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Sep 4, 2019
@openshift-merge-robot openshift-merge-robot merged commit 100cc8a into redhat-developer:master Sep 4, 2019
@kadel kadel mentioned this pull request Sep 4, 2019
Closed
@dharmit dharmit mentioned this pull request Sep 26, 2019
Merged
@rm3l rm3l added the estimated-size/M (10-20) Rough sizing for Epics. About 1 sprint of work for one person label Jun 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dharmit dharmit dharmit approved these changes

@cdrage cdrage cdrage approved these changes

@mohammedzee1000 mohammedzee1000 mohammedzee1000 approved these changes

Assignees

@dharmit dharmit

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. estimated-size/M (10-20) Rough sizing for Epics. About 1 sprint of work for one person lgtm Indicates that a PR is ready to be merged. Required by Prow.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

"odo service create" doesn't set parameters correctly unless used in interactive mode
7 participants
@girishramnani @openshift-ci-robot @dharmit @cdrage @mohammedzee1000 @rm3l @openshift-merge-robot