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.

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

Add attach, run, and annotate to cli #4299

Merged
merged 4 commits into from Aug 23, 2015

Conversation

smarterclayton
Copy link
Contributor

Expose the commands to clients

@smarterclayton
Copy link
Contributor Author

[test]

@liggitt
Copy link
Contributor

liggitt commented Aug 21, 2015

LGTM

@smarterclayton
Copy link
Contributor Author

I have one more commit to make run default to a DC with a fallback to creating an RC.

@smarterclayton
Copy link
Contributor Author

Updated, ready for review

@smarterclayton smarterclayton force-pushed the upstream_commands branch 2 times, most recently from 7c018b1 to 91fe6d4 Compare August 21, 2015 02:40
@deads2k
Copy link
Contributor

deads2k commented Aug 21, 2015

@@ -12,6 +12,13 @@ func init() {
}

err := api.Scheme.AddDefaultingFuncs(
func(obj *DeploymentConfigSpec) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm pretty sure this broke serialization tests. Why are defaulting this?

(Broke in the sense that you need to update them, not in the sense that we should never do this.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we should, going to put it in its own commit.

On Aug 21, 2015, at 8:30 AM, David Eads notifications@github.com wrote:

In pkg/deploy/api/v1/defaults.go
#4299 (comment):

@@ -12,6 +12,13 @@ func init() {
}

err := api.Scheme.AddDefaultingFuncs(

  • func(obj *DeploymentConfigSpec) {
    

I'm pretty sure this broke serialization tests. Why are defaulting this?


Reply to this email directly or view it on GitHub
https://github.com/openshift/origin/pull/4299/files#r37629330.

@smarterclayton
Copy link
Contributor Author

Hrm... kubectlcompattest didn't fail...

@deads2k
Copy link
Contributor

deads2k commented Aug 21, 2015

Hrm... kubectlcompattest didn't fail...

I didn't build the check the other way around because we add a lot of additional subcommands.

@smarterclayton
Copy link
Contributor Author

I'm referring to the negative check of something that says it should be
missing, but isn't. See the 3rd commit.

On Fri, Aug 21, 2015 at 9:21 AM, David Eads notifications@github.com
wrote:

Hrm... kubectlcompattest didn't fail...

I didn't build the check the other way around because we add a lot of
additional subcommands.


Reply to this email directly or view it on GitHub
#4299 (comment).

Clayton Coleman | Lead Engineer, OpenShift

@smarterclayton
Copy link
Contributor Author

If nothing else I'm going to merge this.....

@deads2k
Copy link
Contributor

deads2k commented Aug 21, 2015

lgtm

@smarterclayton
Copy link
Contributor Author

[merge]

@smarterclayton
Copy link
Contributor Author

[test]

1 similar comment
@smarterclayton
Copy link
Contributor Author

[test]

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/3075/) (Image: devenv-fedora_2197)

@openshift-bot
Copy link
Contributor

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

@smarterclayton smarterclayton force-pushed the upstream_commands branch 2 times, most recently from b63a590 to 6eadf9b Compare August 22, 2015 19:08
@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 654eff6

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 654eff6

openshift-bot pushed a commit that referenced this pull request Aug 23, 2015
@openshift-bot openshift-bot merged commit 5a8501b into openshift:master Aug 23, 2015
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.

None yet

4 participants