-
Notifications
You must be signed in to change notification settings - Fork 414
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: add izanami as feature flipping provider #2507
Conversation
engine/api/feature/flipping.go
Outdated
) | ||
|
||
const ( | ||
FeatWorkflowAsCode = "cds:wasc" |
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.
why public? If public, need some doc
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.
Public because you will need it on code
sdk/project.go
Outdated
@@ -31,6 +31,13 @@ type Project struct { | |||
Keys []ProjectKey `json:"keys" yaml:"keys" db:"-" cli:"-"` | |||
VCSServers []ProjectVCSServer `json:"vcs_servers" yaml:"vcs_servers" db:"-" cli:"-"` | |||
Platforms []ProjectPlatform `json:"platforms" yaml:"platforms" db:"-" cli:"-"` | |||
Features ProjectFeatures `json:"features" yaml:"platforms" db:"-" cli:"-"` |
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.
yml platforms --> features
engine/api/api.go
Outdated
ClientSecret string `toml:"client_secret"` | ||
Token string `toml;"token" comment:"Token shared between Izanami and CDS to be able to send webhooks from izanami"` | ||
} | ||
} |
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.
missing doc for toml configuration, and toml key name for FeaturesFlipping and Izanami
engine/api/api.go
Outdated
ClientSecret string `toml:"client_secret"` | ||
Token string `toml;"token" comment:"Token shared between Izanami and CDS to be able to send webhooks from izanami"` | ||
} | ||
} |
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 miss toml comments
engine/api/api.go
Outdated
@@ -352,6 +361,12 @@ func (a *API) Serve(ctx context.Context) error { | |||
a.Config.SMTP.TLS, | |||
a.Config.SMTP.Disable) | |||
|
|||
// Initialize feature package | |||
log.Info("Initializing feature flipping") |
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.
add the URL in the log (kinda a debug the bug)
engine/api/api.go
Outdated
// Initialize feature package | ||
log.Info("Initializing feature flipping") | ||
if a.Config.FeaturesFlipping.Izanami.ApiURL != "" { | ||
feature.Init(a.Config.FeaturesFlipping.Izanami.ApiURL, a.Config.FeaturesFlipping.Izanami.ClientID, a.Config.FeaturesFlipping.Izanami.ClientSecret) |
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.
check the error
"github.com/ovh/cds/engine/api/feature" | ||
) | ||
|
||
func (api *API) cleanFeatureHandler() Handler { |
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.
Please add a test (to cover the router)
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 will be overkill no?
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.
done
is it possible to add |
@yesnault you can create an issue, but do not block PR for this |
engine/api/api.go
Outdated
ClientID string `toml:"client_id"` | ||
ClientSecret string `toml:"client_secret"` | ||
Token string `toml;"token" comment:"Token shared between Izanami and CDS to be able to send webhooks from izanami"` | ||
} |
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.
missing `toml:"izanami" + comment about what is is, why use it, etc... or just 'enable alpha feature, use it with precaution'
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.
and something under https://ovh.github.io/cds/hosting/ about izanami?
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.
not for now, it's not mandatory. We also have to discuss about release ;)
engine/api/api.go
Outdated
@@ -134,7 +134,7 @@ type Configuration struct { | |||
ClientID string `toml:"client_id"` | |||
ClientSecret string `toml:"client_secret"` | |||
Token string `toml;"token" comment:"Token shared between Izanami and CDS to be able to send webhooks from izanami"` |
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.
toml;"token" -> toml:"token"
@ovh/cds