helm: rename status conditional UpdateError to UpgradeError #3269

camilamacedo86 · 2020-06-21T19:57:38Z

Description of the change:

helm: rename status conditional UpdateError to UpgradeError

Motivation for the change:

related to the changes done in the transition to using the new layout.

estroz

Why is this change necessary? If it is, it is definitely a breaking change and need a deprecation strategy.

camilamacedo86 · 2020-06-24T08:50:18Z

Hi @estroz,

The fragment was updated with the info.

camilamacedo86 · 2020-06-24T08:58:11Z

changelog/fragments/helm-conditional.yaml

+        For Helm based-Operator be more aligned with Helm the `UpdateError` was renamed to `UpgradeError`. 
+        Note that it is NOT a breaking change for the Helm based-Operators itself, however, if you have any 
+        script or code implemented which is using this status to perform checks then, will be required replace 
+        `UpdateError` for `UpgradeError` in this places. 


Hi @estroz,

What we are changing here is the status name which will appear in the CR.status to be aligned with helm ( part of the "new" helm-operator ). I agree that users might be checking it due to a script, for example, the status for tests and etc and then, it would be a breaking change for them but not for o Helm based-Operator itself.

My idea/suggestion is we do it before release 1.0.0 image for users is able to move smoothly.
@joelanford wdyt?

joelanford · 2020-06-24T16:23:49Z

changelog/fragments/helm-conditional.yaml

+      The status conditional `UpdateError` was renamed to `UpgradeError` to better align with Helm nomenclature.
+      This change just affects Helm based-operators.


Suggested change

The status conditional `UpdateError` was renamed to `UpgradeError` to better align with Helm nomenclature.

This change just affects Helm based-operators.

In Helm-based operators, the `UpdateError` condition reason was renamed to `UpgradeError` to better align with Helm nomenclature.

joelanford · 2020-06-24T16:25:42Z

changelog/fragments/helm-conditional.yaml

+    # Migration can be defined to automatically add a section to
+    # the migration guide. This is required for breaking changes.
+    migration:
+      header: `UpdateError` status for Helm based-Operator was ranamed 


Suggested change

header: `UpdateError` status for Helm based-Operator was ranamed

header: "Helm: rename condition reason `UpdateError` to `UpgradeError`"

joelanford · 2020-06-24T16:33:32Z

changelog/fragments/helm-conditional.yaml

+      body: >
+        For Helm based-Operator be more aligned with Helm the `UpdateError` was renamed to `UpgradeError`. 
+        Note that it is NOT a breaking change for the Helm based-Operators itself, however, if you have any 
+        script or code implemented which is using this status to perform checks then, will be required replace 
+        `UpdateError` for `UpgradeError` in this places. 


Use the yaml | operator instead of > for the migration body so we can have separate paragraphs for easier readability. We should probably make this change in the fragment template.

Also some general grammar/syntax suggestions.

Suggested change

body: >

For Helm based-Operator be more aligned with Helm the `UpdateError` was renamed to `UpgradeError`.

Note that it is NOT a breaking change for the Helm based-Operators itself, however, if you have any

script or code implemented which is using this status to perform checks then, will be required replace

`UpdateError` for `UpgradeError` in this places.

body: |

For Helm-based operators to be more aligned with Helm, the `UpdateError` condition

reason was renamed to `UpgradeError` for the `ReleaseFailed` condition.

Note that this is **NOT** a breaking change for Helm-based operators themselves.

However, any script or code that is depending on this condition reason must be updated

to use `UpgradeError` instead of `UpdateError`.

joelanford · 2020-06-24T16:41:05Z

@estroz This was an oversight from the very beginning. Helm has always used "upgrade" to refer to releases, not "update". I think this is a good time to get this in before 1.0.

I'm not sure how a deprecation for this would work without us basically duplicating the entire condition and also renaming the condition type from ReleaseFailed to something else.

estroz · 2020-06-24T16:44:58Z

@joelanford @camilamacedo86 I think we have to do a hard break.

estroz

/lgtm

joelanford

/lgtm

mikeshng · 2020-07-06T15:41:30Z

@camilamacedo86

ReasonUpdateSuccessful    HelmAppConditionReason = "UpdateSuccessful"

Hi, is there a discussion on why we are leaving the reason as Update but not Upgrade? Thanks.

camilamacedo86 · 2020-07-06T15:45:09Z

Hi @mikeshng,

Good catcher, we should update the Reason as well. Really tks for the check. It passed. I am doing a pr for that.

…gradeSuccessful #3345 **Description of the change:** - Follow-up of : #3269 - helm: rename status conditional UpdateSuccessful to UpgradeSuccessful **Motivation for the change:** related to the changes done in the transition to using the new layout.

camilamacedo86 requested a review from joelanford June 21, 2020 19:57

openshift-ci-robot requested a review from varshaprasad96 June 21, 2020 19:57

estroz suggested changes Jun 23, 2020

View reviewed changes

camilamacedo86 commented Jun 24, 2020

View reviewed changes

joelanford requested changes Jun 24, 2020

View reviewed changes

helm: rename status conditonal UpdateError to UpgradeError

65a0f7b

camilamacedo86 added this to the v0.19.0 milestone Jun 24, 2020

camilamacedo86 requested review from joelanford and estroz June 24, 2020 16:54

upgrade fragment

3c801fa

estroz approved these changes Jun 24, 2020

View reviewed changes

openshift-ci-robot assigned estroz Jun 24, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 24, 2020

joelanford approved these changes Jun 29, 2020

View reviewed changes

openshift-ci-robot assigned joelanford Jun 29, 2020

camilamacedo86 merged commit 9bf0dbf into operator-framework:master Jun 29, 2020

camilamacedo86 deleted the helm-conditional-up branch June 29, 2020 17:25

camilamacedo86 mentioned this pull request Jul 6, 2020

feat: condition status conditional UpdateSuccessful was renamed to UpgradeSuccessful #3345

Merged

2 tasks

This was referenced Jul 6, 2020

Is there a plan to further refactor Helm Operator references of "update" to "upgrade"? #3346

Closed

helm-operator: use "Upgrade" instead of "Update" in var/fn names #3388

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

helm: rename status conditional UpdateError to UpgradeError #3269

helm: rename status conditional UpdateError to UpgradeError #3269

camilamacedo86 commented Jun 21, 2020

estroz left a comment •

edited

Loading

camilamacedo86 commented Jun 24, 2020 •

edited

Loading

camilamacedo86 Jun 24, 2020

joelanford Jun 24, 2020

joelanford Jun 24, 2020

joelanford Jun 24, 2020

joelanford commented Jun 24, 2020

estroz commented Jun 24, 2020

estroz left a comment

joelanford left a comment

mikeshng commented Jul 6, 2020 •

edited

Loading

camilamacedo86 commented Jul 6, 2020

		The status conditional `UpdateError` was renamed to `UpgradeError` to better align with Helm nomenclature.
		This change just affects Helm based-operators.

	The status conditional `UpdateError` was renamed to `UpgradeError` to better align with Helm nomenclature.
	This change just affects Helm based-operators.
	In Helm-based operators, the `UpdateError` condition reason was renamed to `UpgradeError` to better align with Helm nomenclature.

	header: `UpdateError` status for Helm based-Operator was ranamed
	header: "Helm: rename condition reason `UpdateError` to `UpgradeError`"

-      body: >
-        For Helm based-Operator be more aligned with Helm the `UpdateError` was renamed to `UpgradeError`.
-        Note that it is NOT a breaking change for the Helm based-Operators itself, however, if you have any
-        script or code implemented which is using this status to perform checks then, will be required replace
-        `UpdateError` for `UpgradeError` in this places.
+      body: |
+        For Helm-based operators to be more aligned with Helm, the `UpdateError` condition
+        reason was renamed to `UpgradeError` for the `ReleaseFailed` condition.
+        Note that this is **NOT** a breaking change for Helm-based operators themselves.
+        However, any script or code that is depending on this condition reason must be updated
+        to use `UpgradeError` instead of `UpdateError`.

helm: rename status conditional UpdateError to UpgradeError #3269

helm: rename status conditional UpdateError to UpgradeError #3269

Conversation

camilamacedo86 commented Jun 21, 2020

estroz left a comment • edited Loading

Choose a reason for hiding this comment

camilamacedo86 commented Jun 24, 2020 • edited Loading

camilamacedo86 Jun 24, 2020

Choose a reason for hiding this comment

joelanford Jun 24, 2020

Choose a reason for hiding this comment

joelanford Jun 24, 2020

Choose a reason for hiding this comment

joelanford Jun 24, 2020

Choose a reason for hiding this comment

joelanford commented Jun 24, 2020

estroz commented Jun 24, 2020

estroz left a comment

Choose a reason for hiding this comment

joelanford left a comment

Choose a reason for hiding this comment

mikeshng commented Jul 6, 2020 • edited Loading

camilamacedo86 commented Jul 6, 2020

estroz left a comment •

edited

Loading

camilamacedo86 commented Jun 24, 2020 •

edited

Loading

mikeshng commented Jul 6, 2020 •

edited

Loading