Skip to content

apps: add DeploymentConditionReason to api#37

Closed
mfojtik wants to merge 1 commit into
openshift:masterfrom
mfojtik:consts
Closed

apps: add DeploymentConditionReason to api#37
mfojtik wants to merge 1 commit into
openshift:masterfrom
mfojtik:consts

Conversation

@mfojtik
Copy link
Copy Markdown
Contributor

@mfojtik mfojtik commented Mar 5, 2018

Ref: openshift/origin#18269

This adds DeploymentConditionReason as API type, replacing the string type for DeploymentCondition Reason field.

/cc @tnozicka

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 5, 2018
@mfojtik
Copy link
Copy Markdown
Contributor Author

mfojtik commented Mar 5, 2018

@deads2k PTAL

@deads2k
Copy link
Copy Markdown
Contributor

deads2k commented Mar 5, 2018

lgtm.

I'll leave tagging to @liggitt because every pull that makes master differ, but isn't reconciled before a rebase is more risk of something breaking during the rebase.

@liggitt
Copy link
Copy Markdown
Contributor

liggitt commented Mar 5, 2018

let's hold purely cosmetic changes like this

Comment thread apps/v1/types.go
ReplicationControllerUpdatedReason DeploymentConditionReason = "ReplicationControllerUpdated"
// FailedRcCreateReason is added in a deployment config when it cannot create a new replication
// controller.
FailedRcCreateReason DeploymentConditionReason = "ReplicationControllerCreateError"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: RC (also, is there a reason to name the constant so differently from the value? these become public API and shouldn't change if we can avoid it)

Comment thread apps/v1/types.go
// NewRcAvailableReason is added in a deployment config when its newest replication controller is made
// available ie. the number of new pods that have passed readiness checks and run for at least
// minReadySeconds is at least the minimum available pods that need to run for the deployment config.
NewRcAvailableReason DeploymentConditionReason = "NewReplicationControllerAvailable"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

harmonize constant name with actual value

@liggitt
Copy link
Copy Markdown
Contributor

liggitt commented Apr 9, 2018

in general, it is easier to find the constants you need if the variable names align with the values

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 26, 2018
@deads2k deads2k self-assigned this Jul 9, 2018
Comment thread apps/v1/types.go
TimedOutReason DeploymentConditionReason = "ProgressDeadlineExceeded"
// PausedConfigReason is added in a deployment config when it is paused. Lack of progress shouldn't be
// estimated once a deployment config is paused.
PausedConfigReason DeploymentConditionReason = "DeploymentConfigPaused"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't see this set. What sets it?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't see this set. What sets it?

in origin I mean

@deads2k
Copy link
Copy Markdown
Contributor

deads2k commented Jul 9, 2018

in general, it is easier to find the constants you need if the variable names align with the values

And double check usage.

@mfojtik
Copy link
Copy Markdown
Contributor Author

mfojtik commented Jul 11, 2018

And double check usage.

@deads2k that is what I do in openshift/origin#20019

I moved all constants that we want to have in openshift/api into "pkg/apps/util" for now. These are used by both CLI and controllers/strategies so we should have them in a common library/package where clients can read them (things like status/timeouts/etc.).

@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 19, 2018
@mfojtik
Copy link
Copy Markdown
Contributor Author

mfojtik commented Aug 23, 2018

Closing in favor of #89

@mfojtik mfojtik closed this Aug 23, 2018
openshift-cherrypick-robot pushed a commit to openshift-cherrypick-robot/api that referenced this pull request Feb 1, 2021
pkg/manifests: use yaml decoder to avoid conversion errors
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants