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

[WIP] Add separate call for updating DC status #6479

Closed
wants to merge 1 commit into from
Closed

[WIP] Add separate call for updating DC status #6479

wants to merge 1 commit into from

Conversation

0xmichalis
Copy link
Contributor

Blocked on #6337

@ironcladlou @deads2k

Needs a couple of unit tests.

Fixes #6465

@0xmichalis
Copy link
Contributor Author

oc deploy --latest works fine: http://pastebin.com/ynGZvrfR

@@ -222,7 +222,7 @@ func (o DeployOptions) deploy(config *deployapi.DeploymentConfig, out io.Writer)
}

config.Status.LatestVersion++
_, err = o.osClient.DeploymentConfigs(config.Namespace).Update(config)
_, err = o.osClient.DeploymentConfigs(config.Namespace).UpdateStatus(config)
Copy link
Contributor

Choose a reason for hiding this comment

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

is this backwards compatible?

Copy link
Contributor

Choose a reason for hiding this comment

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

actually, we would never want a client to modify status to trigger a deployment... a client would modify spec, the controller would modify status to indicate the requested deployment was done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is #6337 for fixing that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't want a version of the CLI to exist which makes a call to the status subresource in order to trigger a deployment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. Then we need #6337 to be fixed first.

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW in the short term, updates to the status won't result in a new deployment unless status.latestVersion changes.

@openshift-bot
Copy link
Contributor

Origin Action Required: Pull request cannot be automatically merged, please rebase your branch from latest HEAD and push again

@openshift-bot openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 1, 2016
This commit adds a separete call for updating the status part of a
DeploymentConfig.
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 8, 2016
@0xmichalis
Copy link
Contributor Author

Superseded by #7149

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