Skip to content

tolerate multiple =s in a parameter value#10880

Merged
openshift-bot merged 1 commit intoopenshift:masterfrom
bparees:fix_process_parames
Sep 12, 2016
Merged

tolerate multiple =s in a parameter value#10880
openshift-bot merged 1 commit intoopenshift:masterfrom
bparees:fix_process_parames

Conversation

@bparees
Copy link
Contributor

@bparees bparees commented Sep 12, 2016

@stevekuznetsov @smarterclayton this was introduced in 6eaf7a2 (thanks @fabianofranz for digging it up).

it seems like a fairly bad regression (https://bugzilla.redhat.com/show_bug.cgi?id=1375275)

so if we're still doing merges to 3.3, i'd say this is a candidate.

@bparees bparees force-pushed the fix_process_parames branch from ce0e2e9 to 796eac4 Compare September 12, 2016 18:43
@bparees
Copy link
Contributor Author

bparees commented Sep 12, 2016

[test]

@stevekuznetsov
Copy link
Contributor

@bparees can we add a test case to /test/cmd/process.sh?

@smarterclayton
Copy link
Contributor

It's a regression but probably missed the train unless we respin. I'll pull it to 1.3.0 origin though.

@smarterclayton
Copy link
Contributor

Add the test case and I'll review and merge.

@smarterclayton smarterclayton added this to the 1.3.0 milestone Sep 12, 2016
@bparees bparees force-pushed the fix_process_parames branch from 796eac4 to 4b0223e Compare September 12, 2016 20:09
@bparees
Copy link
Contributor Author

bparees commented Sep 12, 2016

@smarterclayton @stevekuznetsov TC added. Note I've also now fixed an issue where if you supplied a value of the form "--value=key", we panicked because we'd try to reference the second element of the split result, even if the split hadn't returned 2+ values.

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 4b0223e

@smarterclayton
Copy link
Contributor

LGTM [merge]

os::cmd::expect_failure_and_text "oc process -f '${required_params}' --value=required_param=someval --value=other_param=otherval" 'unknown parameter name "other_param"'
# failure on values fails the entire call
os::cmd::expect_failure_and_text "oc process -f '${required_params}' --value=required_param=someval --value=optional_param=some=series=of=values=" 'invalid parameter assignment in'
os::cmd::expect_failure_and_text "oc process -f '${required_params}' --value=required_param=someval --value=optional_param" 'invalid parameter assignment in'
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this test doing now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same thing it did before. ensuring that if you supply an invalid parameter arg, the whole call fails w/ the correct error message. it's just that now it's incorrect because there's no equal sign as part of the arg, instead of before when it was incorrect because there were multiple equals in the arg.

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 4b0223e

@openshift-bot
Copy link
Contributor

openshift-bot commented Sep 12, 2016

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/8817/) (Image: devenv-rhel7_5003)

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/8816/)

@openshift-bot openshift-bot merged commit 3cd9a1f into openshift:master Sep 12, 2016
@bparees bparees deleted the fix_process_parames branch September 15, 2016 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants