-
Notifications
You must be signed in to change notification settings - Fork 113
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
Add update strategy to the channel badge #839
Conversation
Signed-off-by: Salim Afiune <afiune@chef.io>
Signed-off-by: Salim Afiune <afiune@chef.io>
Signed-off-by: Salim Afiune <afiune@chef.io>
Signed-off-by: Salim Afiune <afiune@chef.io>
Signed-off-by: Salim Afiune <afiune@chef.io>
Signed-off-by: Salim Afiune <afiune@chef.io>
// exactly the same, we do care about order but in some degree, for example health check | ||
// status and fqdn, but we don't care about things like order of supervisor_id so we can | ||
// skip that check by not specifying it into the expected response | ||
func assertServicesEqual(t *testing.T, expected, actual []*applications.Service) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ assert.ElementsMatch(t, expected, actual)
doesn't do the trick?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First of all, thanks for looking into this PR! Second, this was just a move of code from A to B file 06b87a1
Third and last, no, it doesn't do the trick since we might have db IDs and timestamps that won't match exactly the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some nitpicks
@@ -126,6 +129,25 @@ func main() { | |||
clientID = "applications-publisher-" + strconv.FormatInt(t.UnixNano(), 10) | |||
} | |||
|
|||
// If the channel was specified, add the update config | |||
if len(channel) > 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probably fine, but if no channel is provided, but an update-strategy, the latter will just be ignored; it won't raise an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@srenatus not sure if you noticed but, we default the strategy to at-once
, but the important thing here is that channel, we want to replicate the behavior of habitat and, if in Habitat you don't specify a channel, then the strategy won't be populated. TL DR no channel, no strategy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, what I imagined to possibly happen is this:
When I call applications-publisher --channel dev --update-strategy foobar
, I get an error. When I run applications-publisher --update-strategy foobar
, I don't. In a perfect world, specifying --update-strategy
without specifying --channel
would perhaps be an error.
However, I don't know if our cli arg lib makes this simple; if it doesn't, I doubt it's worth it 🤷♂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, well I can add this check for sure. At the end, this is just a publisher tool to populate our DB but we want it to be as similar as a habitat supervisor as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your call -- I have to little context to discern if this an issue. Probably not 😄
@@ -58,6 +88,7 @@ type Service struct { | |||
Channel string | |||
Site string | |||
PreviousHealth string `db:"previous_health"` | |||
UpdateStrategy string `db:"update_strategy"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little surprised about db struct tags here; in pkg/storage/postgres
, I understand them. Here? Not really.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I might have to fix that in a next PR. I agree with what you are mentioning here!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -106,6 +106,7 @@ type service struct { | |||
Channel string `db:"channel"` | |||
FullPkgIdent string `db:"package_ident"` | |||
PreviousHealth string `db:"previous_health"` | |||
UpdateStrategy string `db:"update_strategy"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose we could also use a PG ENUM here. But I know it's cumbersome to do all the translation... (we've got example code for this in authz-service, but using database/sql
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would it be the benefits of using an ENUM in postgres?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same as your storage layer's enum, I guess, just one layer down. Anyways, in authlahoma, we've stopped doing that, as it's cumbersome to deal with enums on every layer. 🙄
@@ -7,6 +7,7 @@ import ( | |||
|
|||
"github.com/chef/automate/api/external/applications" | |||
"github.com/chef/automate/api/external/habitat" | |||
"github.com/chef/automate/components/applications-service/pkg/storage" | |||
"github.com/go-gorp/gorp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] It's nice if the imports are sorted; first stdlib, then third-party deps, then internal deps. goimports -local github.com/chef/automate
supports that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am supposedly running that command in my vim editor, not sure why it is not sorting them! 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this how you want them¿?
import (
"fmt"
"strings"
"time"
dblib "github.com/chef/automate/lib/db"
"github.com/go-gorp/gorp"
"github.com/golang/protobuf/ptypes"
timestamp "github.com/golang/protobuf/ptypes/timestamp"
"github.com/pkg/errors"
"github.com/prometheus/client_golang/prometheus"
"github.com/prometheus/client_golang/prometheus/promauto"
log "github.com/sirupsen/logrus"
"github.com/chef/automate/api/external/applications"
"github.com/chef/automate/api/external/habitat"
"github.com/chef/automate/components/applications-service/pkg/storage"
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💭 I'd cluster the dblib with the rest in the last section, but it's also a bit of a nitpicky comment anyways -- 👍
@@ -19,6 +19,7 @@ export interface Service { | |||
fqdn: string; | |||
site: string; | |||
channel: string; | |||
update_strategy: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could use a string literal type, I guess? update_strategy: "NONE" | "ROLLING" | "AT_ONCE";
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: It is AT-ONCE
, but I like that idea!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, you get the gist 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Signed-off-by: Salim Afiune <afiune@chef.io>
Signed-off-by: Salim Afiune <afiune@chef.io>
Signed-off-by: Salim Afiune <afiune@chef.io>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering if the enum way of specifying the update strategy might make some possible upgrade scenarios between habitat supervisor and Automate a little more difficult.
type UpdateStrategy int | ||
|
||
const ( | ||
NoneStrategy UpdateStrategy = iota |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💭 could this cause an issue if, say habitat adds a new kind of upgrade strategy and the user adopts the new upgrade strategy before they upgrade automate to a newer version that has this "enum" updated? If we really want this to be an enum then maybe 0
should be "Unknown" or something like that(?). "NONE" implies that the supervisor won't update following a package promotion, which could affect a use case like "find all the services that will update if I promote package X to the "production" channel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. When habitat has a new upgrade strategy, the habitat team has to also update the protobuf enum defined in the event messages, that is where automate and habitat won’t be in sync and we need to update our protobuf and code to accept the new strategy.
Going deeper into this topic, any modification that habitat will do will also trigger a need for us to update our protobuf and code. Unless we code to accept anything that comes through, which has its trade offs.
What would you like do to here? Remove the enum or continue having it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great on the UI!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Signed-off-by: Salim Afiune <afiune@chef.io>
Signed-off-by: Salim Afiune <afiune@chef.io>
Talked with @apriofrost and @danielsdeleo about those cases where Habitat could introduce a new update strategy and Automate is not capable to understand it, an example is a customer that upgrade Habitat to the latest version with a new update strategy but not upgrade Automate, in such cases we would like to create a new I will work on this in a separate PR since it will be simpler and smaller for review. |
Signed-off-by: Salim Afiune <afiune@chef.io>
🔩 Description
The update strategy tells users how the services will be updated among a group. Habitat supports three update strategies:
none
,rolling
, andat-once
.Showing the data along with the channel name helps users understand where the service will get an update from, and how it will update.
👍 Definition of Done
Include the update strategy for a service instance in the channel badge, in the format of
[channel name]: [update strategy name]
(there is a space after the:
)When there is NO channel and NO update strategy, display
NO CHANNEL: NONE
Mockup
👟 Demo Script / Repro Steps
automate-gateway
,applications-service
, and theautomate-ui
.start_all_services
.-- or
⛓️ Related Resources
https://chefio.atlassian.net/browse/A2-887 & #629
✅ Checklist