apps: add missing consts and vars by mfojtik · Pull Request #89 · openshift/api

mfojtik · 2018-08-23T08:24:46Z

This adds missing consts and vars. The name of consts are harmonized to match the values.

/cc @openshift/api-review

tnozicka · 2018-08-23T09:46:09Z

+	DeployerPodStartedAtAnnotation    = "openshift.io/deployer-pod.started-at"
+	DeployerPodCompletedAtAnnotation  = "openshift.io/deployer-pod.completed-at"
+	DeploymentReplicasAnnotation      = "openshift.io/deployment.replicas"
+	DesiredReplicasAnnotation         = "kubectl.kubernetes.io/desired-replicas"


I don't think ~~are~~ openshift API is a good match for this prefix

I don't think are API is a good match for this prefix

I assumed that this is an indication of what is, not what will be. I would like a code reference though. I thought this was only done by/for the rollout command.

defining kubectl.kubernetes.io in openshift API

There's nothing wrong with defining the constant here. The point of a constant is that you can copy them anywhere, because they have to be constant.

tnozicka · 2018-08-23T09:47:56Z

+	DeployerPodCreatedAtAnnotation    = "openshift.io/deployer-pod.created-at"
+	DeployerPodStartedAtAnnotation    = "openshift.io/deployer-pod.started-at"
+	DeployerPodCompletedAtAnnotation  = "openshift.io/deployer-pod.completed-at"
+	DeploymentReplicasAnnotation      = "openshift.io/deployment.replicas"


This one seems internal, I am not even sure from the top of my head why we have it

well, I don't see it used by anything ;-)

I see it being set, not used

I see it being set, not used

Set where? Leave it out of this pull for now then.

https://github.com/openshift/origin/blob/610fb485858ddab97d8dc35b08ddb4b809b47780/pkg/apps/util/deployment.go#L100
https://github.com/openshift/origin/blob/610fb485858ddab97d8dc35b08ddb4b809b47780/pkg/apps/controller/deployer/deployer_controller.go#L320

Wow, isn't that set incorrectly?

Add a TODO to revisit its meaning.

I don't think we need that annotation at all, nor should upstream (they are actively using similar one). If there is a change and you see the DC template and the RC it seems pretty straight forward to distinguish a scale update from config update.

Don't add it, I am gonna drop its occurrences.

tnozicka · 2018-08-23T09:50:13Z

 	From corev1.ObjectReference `json:"from" protobuf:"bytes,1,opt,name=from"`
 }

+type DeploymentStatus string


we were usually keeping those close to the enum definition, is this a new pattern in opeshift/api?

it is types vs. const I don't feel strong about where this definition will live

These two parts form an enum in Go. Defining an enum in 2 files seems weird to me; maybe that's by my notion of enums from stronger types languages like C++.

it is types vs. const I don't feel strong about where this definition will live

I think @tnozicka is correct. We consistently place it here to help those without IDEs.

No problem, I will move it

deads2k · 2018-08-23T12:22:09Z

@@ -0,0 +1,71 @@
+package v1
+
+const (


@mfojtik can you break these out into which objects they're placed on and see about some doc? Something like "this set of annotations are set on a replicationcontroller created by the DC controller".

It will help us know which are inputs and which are outputs.

sure, it will take some time to do that investigation, but I guess it is worth

@deads2k done. added comments for each const and also brief desc of what sets it and what consume it.

mfojtik · 2018-08-30T10:32:07Z

+	// records the time in RFC3339 format of when the deployer pod for this particular
+	// deployment was started.
+	// This is set by deployer controller, but not consumed by any command or internally.
+	DeployerPodStartedAtAnnotation = "openshift.io/deployer-pod.started-at"


@tnozicka @deads2k candidate for removal

why? or you just want to move it to appsutil?

@tnozicka I think these are providing "future" value for us, not for users or anyone integrating with OpenShift... I think the intention here was to improve the deployer process by knowing the times here so we can calculate the timeouts better. That to me is an internal thing.

@tnozicka yes, appsutil

+1, appsutil only since it's internal

mfojtik · 2018-08-30T10:32:21Z

+	// records the time in RFC3339 format of when the deployer pod for this particular
+	// deployment was created.
+	// This is set by deployer controller, but not consumed by any command or internally.
+	DeployerPodCreatedAtAnnotation = "openshift.io/deployer-pod.created-at"


@tnozicka @deads2k candidate to removal

mfojtik · 2018-08-30T10:32:37Z

+
+	// DeployerPodCompletedAtAnnotation is an annotation on deployment that records
+	// the time in RFC3339 format of when the deployer pod finished.
+	// This is set by deployer controller, but not consumed by any command or internally.


@tnozicka @deads2k candidate for removal

mfojtik · 2018-08-30T10:32:47Z

+	// DesiredReplicasAnnotation represents the desired number of replicas for a
+	// new deployment.
+	// This is set by deployer controller, but not consumed by any command or internally.
+	DesiredReplicasAnnotation = "kubectl.kubernetes.io/desired-replicas"


@tnozicka @deads2k candidate for removal

+1, I'd remove it from the whole code base

@tnozicka will make PR

tnozicka · 2018-08-30T10:37:45Z

+const (
+	// DeploymentStatusReasonAnnotation represents the reason for deployment being in a given state
+	// Used for specifying the reason for cancellation or failure of a deployment
+	// This is set by deployer controller and consumed in retry and history commands.


I am slightly afraid such comments get outdated over time

they are for review only ;-) i will remove them before merge

mfojtik · 2018-08-30T11:28:27Z

+	// DeploymentAnnotation is an annotation on a deployer Pod. The annotation value is the name
+	// of the deployment (a ReplicationController) on which the deployer Pod acts.
+	// This is set by deployer controller and consumed internally and in oc adm top command.
+	DeploymentAnnotation = "openshift.io/deployment.name"


This is really a replication controller name annotation inside deployer pod... We already have ownerRef there so I vote for removing this from api and replacing the origin code to use ownerRefs over this annotation.

This is really a replication controller name annotation inside deployer pod... We already have ownerRef there so I vote for removing this from api and replacing the origin code to use ownerRefs over this annotation.

Deprecate this one too.

mfojtik · 2018-08-30T11:30:08Z

+	// DeploymentConfigAnnotation is an annotation name used to correlate a deployment with the
+	// DeploymentConfig on which the deployment is based.
+	// This is set by deployer controller and consumed internally and in oc adm prune command.
+	DeploymentConfigAnnotation = "openshift.io/deployment-config.name"


I suspect this one will be the same as above...

deads2k · 2018-08-30T12:20:33Z

+
+	// DeploymentCancelledAnnotation indicates that the deployment has been cancelled
+	// The annotation value does not matter and its mere presence indicates cancellation.
+	// This is set by deployment config controller or oc rollout cancel command.


set on an RC or a DC?

RC, I will write it there

deads2k · 2018-08-30T12:40:22Z

/lgtm

openshift-ci-robot · 2018-08-30T12:40:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, mfojtik

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [deads2k]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot requested a review from a team August 23, 2018 08:24

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 23, 2018

mfojtik mentioned this pull request Aug 23, 2018

apps: add DeploymentConditionReason to api #37

Closed

tnozicka reviewed Aug 23, 2018

View reviewed changes

deads2k reviewed Aug 23, 2018

View reviewed changes

mfojtik force-pushed the add-template-annotations branch from 6f6f8c6 to 1a3901b Compare August 30, 2018 10:30

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 30, 2018

mfojtik commented Aug 30, 2018

View reviewed changes

tnozicka reviewed Aug 30, 2018

View reviewed changes

mfojtik commented Aug 30, 2018

View reviewed changes

deads2k reviewed Aug 30, 2018

View reviewed changes

apps: add missing consts and vars

bab084e

mfojtik force-pushed the add-template-annotations branch from 1a3901b to bab084e Compare August 30, 2018 12:31

openshift-ci-robot assigned deads2k Aug 30, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 30, 2018

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 30, 2018

openshift-merge-robot merged commit 6dc9c18 into openshift:master Aug 30, 2018

tnozicka mentioned this pull request Sep 5, 2018

api: published field deploymentconfig.status.conditions.reason is not typed openshift/origin#18269

Closed

mfojtik deleted the add-template-annotations branch September 5, 2018 21:10

Conversation

mfojtik commented Aug 23, 2018

Uh oh!

tnozicka Aug 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnozicka Aug 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnozicka Aug 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

tnozicka Aug 23, 2018 •

edited

Loading

tnozicka Aug 29, 2018 •

edited

Loading

tnozicka Aug 23, 2018 •

edited

Loading