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

Keep plan name in instance aggregatedStatus when plan is terminal #1451

Merged
merged 1 commit into from
Apr 2, 2020

Conversation

zen-dog
Copy link
Contributor

@zen-dog zen-dog commented Mar 31, 2020

Summary:
we were resetting Status.AggregatedStatus.ActivePlanName field when the plan was terminal. This lead to this field looking like this:

Status:
  Aggregated Status:
    Status:  COMPLETE

The idea of the field is to keep information about the last/current plan including plan name and status.

Signed-off-by: Aleksey Dukhovniy alex.dukhovniy@googlemail.com

Summary:
we were resetting `Status.AggregatedStatus.ActivePlanName` field when the plan was terminal. This lead to this field looking like this:
```
Status:
  Aggregated Status:
    Status:  COMPLETE
```
The idea of the field is to keep information of the last/current plan including plan name and status.

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
@zen-dog zen-dog changed the title Keep plan name in instance aggregated status when plan is terminal Keep plan name in instance aggregatedStatus when plan is terminal Mar 31, 2020
Copy link
Member

@nfnt nfnt left a comment

Choose a reason for hiding this comment

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

On the other hand, the field is named ActivePlanName. I understand why this is set to empty string if no plan is active. Let's rename this to LastActivePlanName if we want to change the meaning of this field. WDYT?

@zen-dog
Copy link
Contributor Author

zen-dog commented Apr 1, 2020

if we want to change the meaning of this field. WDYT?

I agree with a confusing name, however, introducing a breaking change just to rename a field is unnecessary. Also, I'm planning to remove the AggregateStatus altogether after #1442 lands so 🤷‍♂

Copy link
Member

@kensipe kensipe left a comment

Choose a reason for hiding this comment

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

based on conversation on PR... lgtm... seems like it is short term

@kensipe kensipe merged commit 82511c3 into master Apr 2, 2020
@kensipe kensipe deleted the ad/fix-aggregated-status branch April 2, 2020 13:46
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