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

feat(events): add maintenance events #356

Merged
merged 12 commits into from
Sep 21, 2023
Merged

Conversation

SCedricThomas
Copy link
Contributor

@SCedricThomas SCedricThomas commented Jul 28, 2023

fix #354
fix #357
fix #358

@SCedricThomas SCedricThomas self-assigned this Jul 28, 2023
@yohann-bacha yohann-bacha self-assigned this Sep 12, 2023
@yohann-bacha yohann-bacha marked this pull request as ready for review September 12, 2023 14:43
Copy link
Member

@curzolapierre curzolapierre left a comment

Choose a reason for hiding this comment

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

By looking the event names, I'm wonder, did you succeed to test locally?

There are some changes that need to be done, probably on API side according to the user information, otherwise it should be good.

CHANGELOG.md Outdated
@@ -2,6 +2,8 @@

## To Be Released

* feat(events): add maintenance events"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* feat(events): add maintenance events"
* feat(events): add maintenance events

Comment on lines 873 to 882
func (ev *EventDatabaseMaintenancePlannedType) String() string {
return fmt.Sprintf("A maintenance has been scheduled on the %s addon (Maintenance ID: %s).", ev.TypeData.AddonName, ev.TypeData.MaintenanceID)
}

func (ev *EventDatabaseMaintenancePlannedType) Who() string {
if ev.TypeData.AddonName != "" {
return fmt.Sprintf("Addon %s", ev.TypeData.AddonName)
}
return ev.Event.Who()
}
Copy link
Member

Choose a reason for hiding this comment

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

From slack discussion I talked more about the following format:

A maintenance (ID 65004006a8acb50076cf9c1b) has been scheduled on the Redis addon. <scalingo-platform (deploy@scalingo.com)>

Two things:

  • non-blocking: in order to make as small as possible the output, move the "Maintenance ID" right after "maintenance" using only (ID: 123)
  • blocking: The user is missing here

Copy link
Contributor

Choose a reason for hiding this comment

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

I updated the message format, but the user is well added here. Just not handled in this file.

Comment on lines 895 to 904
func (ev *EventDatabaseMaintenanceStartedType) String() string {
return fmt.Sprintf("A maintenance has started on the %s addon (Maintenance ID: %s).", ev.TypeData.AddonName, ev.TypeData.MaintenanceID)
}

func (ev *EventDatabaseMaintenanceStartedType) Who() string {
if ev.TypeData.AddonName != "" {
return fmt.Sprintf("Addon %s", ev.TypeData.AddonName)
}
return ev.Event.Who()
}
Copy link
Member

Choose a reason for hiding this comment

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

Same as above

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed!

Comment on lines 917 to 926
func (ev *EventDatabaseMaintenanceCompletedType) String() string {
return fmt.Sprintf("A maintenance has been completed on the %s addon (Maintenance ID: %s).", ev.TypeData.AddonName, ev.TypeData.MaintenanceID)
}

func (ev *EventDatabaseMaintenanceCompletedType) Who() string {
if ev.TypeData.AddonName != "" {
return fmt.Sprintf("Addon %s", ev.TypeData.AddonName)
}
return ev.Event.Who()
}
Copy link
Member

Choose a reason for hiding this comment

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

Same as above

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed as well

Comment on lines 145 to 147
EventDatabaseMaintenancePlanned EventTypeName = "database_maintenance_planned"
EventDatabaseMaintenanceStarted EventTypeName = "database_maintenance_started"
EventDatabaseMaintenanceCompleted EventTypeName = "database_maintenance_completed"
Copy link
Member

Choose a reason for hiding this comment

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

These are not the right event names. You can find them from the API event.rake file.

@curzolapierre curzolapierre removed their assignment Sep 12, 2023
@yohann-bacha yohann-bacha marked this pull request as draft September 13, 2023 08:17
@yohann-bacha yohann-bacha changed the title feat(event): add event structs feat(event): add event structs [STORY64 STORY-73 STORY-74] Sep 13, 2023
@yohann-bacha yohann-bacha changed the title feat(event): add event structs [STORY64 STORY-73 STORY-74] feat(event): add event structs [STORY-64 STORY-73 STORY-74] Sep 13, 2023
@curzolapierre curzolapierre removed their request for review September 13, 2023 16:02
@yohann-bacha yohann-bacha marked this pull request as ready for review September 14, 2023 14:52
@yohann-bacha
Copy link
Contributor

image
Here is how it looks like in the CLI. There is still the user problem to fix.

Copy link
Member

@curzolapierre curzolapierre left a comment

Choose a reason for hiding this comment

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

Nice, only need to take in account recent API changes and we'll ready to go 👍

Comment on lines 864 to 865
AddonName string `json:"addon_name"`
MaintenanceID string `json:"maintenance_id"`
Copy link
Member

Choose a reason for hiding this comment

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

Since the last update of the API we probably want to add all available information,
i.e. adding:

  • maintenance_type
  • maintenance_duration
  • maintenance_duration

Comment on lines 878 to 880
if ev.TypeData.AddonName != "" {
return fmt.Sprintf("Database %s", ev.TypeData.AddonName)
}
Copy link
Member

Choose a reason for hiding this comment

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

We want to use the user from the payload. Either remove the condition completely or change the condition to fallback, as you wish.

Copy link
Member

Choose a reason for hiding this comment

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

You didn't take this comment into account

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed

Comment on lines 886 to 887
AddonName string `json:"addon_name"`
MaintenanceID string `json:"maintenance_id"`
Copy link
Member

Choose a reason for hiding this comment

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

Same as above

Comment on lines 900 to 902
if ev.TypeData.AddonName != "" {
return fmt.Sprintf("Database %s", ev.TypeData.AddonName)
}
Copy link
Member

Choose a reason for hiding this comment

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

Same as above

Copy link
Member

Choose a reason for hiding this comment

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

You didn't take this comment into account

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed as well

Comment on lines 908 to 909
AddonName string `json:"addon_name"`
MaintenanceID string `json:"maintenance_id"`
Copy link
Member

Choose a reason for hiding this comment

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

Same as above

Comment on lines 922 to 924
if ev.TypeData.AddonName != "" {
return fmt.Sprintf("Database %s", ev.TypeData.AddonName)
}
Copy link
Member

Choose a reason for hiding this comment

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

Same as above

Copy link
Member

Choose a reason for hiding this comment

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

You didn't take this comment into account

Copy link
Member

@curzolapierre curzolapierre left a comment

Choose a reason for hiding this comment

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

The user is still the addon name. We want to use the one from the payload

Comment on lines 878 to 880
if ev.TypeData.AddonName != "" {
return fmt.Sprintf("Database %s", ev.TypeData.AddonName)
}
Copy link
Member

Choose a reason for hiding this comment

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

You didn't take this comment into account

Comment on lines 900 to 902
if ev.TypeData.AddonName != "" {
return fmt.Sprintf("Database %s", ev.TypeData.AddonName)
}
Copy link
Member

Choose a reason for hiding this comment

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

You didn't take this comment into account

Comment on lines 922 to 924
if ev.TypeData.AddonName != "" {
return fmt.Sprintf("Database %s", ev.TypeData.AddonName)
}
Copy link
Member

Choose a reason for hiding this comment

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

You didn't take this comment into account

@yohann-bacha
Copy link
Contributor

Here is a screen shot of the CLI:
image

Copy link
Member

@curzolapierre curzolapierre left a comment

Choose a reason for hiding this comment

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

Just a remaining typo and we are good to go

@@ -855,3 +858,69 @@ func (ev *EventStackChangedType) String() string {
d := ev.TypeData
return fmt.Sprintf("Stack changed from '%s' to %s", d.PreviousStackName, d.CurrentStackName)
}

// Database maintenance planned
type EventPlanDatabaseMaintenancedTypeData struct {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
type EventPlanDatabaseMaintenancedTypeData struct {
type EventPlanDatabaseMaintenanceTypeData struct {

Copy link
Contributor

Choose a reason for hiding this comment

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

oh damn you're completely right, fixed


type EventPlanDatabaseMaintenanceType struct {
Event
TypeData EventPlanDatabaseMaintenancedTypeData `json:"type_data"`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
TypeData EventPlanDatabaseMaintenancedTypeData `json:"type_data"`
TypeData EventPlanDatabaseMaintenanceTypeData `json:"type_data"`

Copy link
Contributor

Choose a reason for hiding this comment

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

same, fixed

@yohann-bacha yohann-bacha enabled auto-merge (squash) September 21, 2023 10:12
@yohann-bacha yohann-bacha merged commit ca178da into master Sep 21, 2023
2 checks passed
@yohann-bacha yohann-bacha deleted the feat/354/maintenance-events branch September 21, 2023 12:28
@EtienneM EtienneM changed the title feat(event): add event structs [STORY-64 STORY-73 STORY-74] feat(events): add maintenance events Sep 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants