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: config deprecation #2015

Merged
merged 10 commits into from
Feb 23, 2023
Merged

feat: config deprecation #2015

merged 10 commits into from
Feb 23, 2023

Conversation

mathnogueira
Copy link
Member

This PR allows us to deprecate configuration fields and notify the user about them.

Screenshot from 2023-02-17 17-02-29

Checklist

  • tested locally
  • added new dependencies
  • updated the docs
  • added a test

@mathnogueira mathnogueira changed the base branch from main to new-config February 17, 2023 20:02
Comment on lines +8 to +15
{
key: "postgresConnString",
defaultValue: "",
description: "Postgres connection string",
validate: nil,
deprecated: true,
deprecationMessage: "Use the new postgres config structure instead.",
},
Copy link
Member Author

Choose a reason for hiding this comment

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

Added this so we have a warning about this setting.

@danielbdias danielbdias self-requested a review February 17, 2023 20:11
Copy link
Collaborator

@schoren schoren left a comment

Choose a reason for hiding this comment

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

Great work! I think the test relying on writing files to disk needs to be improved, but I like the new apprach!

@@ -31,11 +31,17 @@ type Config struct {
mu sync.Mutex
}

type Logger interface {
Copy link
Collaborator

Choose a reason for hiding this comment

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

you can unexport this, I don't think any external clients will depend on this

@@ -177,3 +185,31 @@ func (c *Config) AnalyticsEnabled() bool {

return c.config.GA.Enabled
}

func warnAboutDeprecatedFields(vp *viper.Viper, logger Logger) error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice name!

func TestDeprecatedOptions(t *testing.T) {
testCases := []deprecatedOptionTest{
{
name: "should_not_detect_any_deprecated_config",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it ok if we noramlize tests naming in CamelCase? so we follow the convention from the test func

server/config/config.go Outdated Show resolved Hide resolved
Comment on lines 15 to 99
type deprecatedOptionTest struct {
name string
fileContent string
expectedDeprecatedKeys []string
}

type logger struct {
messages []string
}

func (l *logger) Println(in ...any) {
message := fmt.Sprintf("%s", in...)
l.messages = append(l.messages, message)
}

func (l *logger) GetMessages() []string {
return l.messages
}

func TestDeprecatedOptions(t *testing.T) {
testCases := []deprecatedOptionTest{
{
name: "should_not_detect_any_deprecated_config",
expectedDeprecatedKeys: []string{},
fileContent: `
postgres:
host: localhost
port: 5432
`,
},
{
name: "should_detect_any_deprecated_config",
expectedDeprecatedKeys: []string{"postgresConnString"},
fileContent: `
postgresConnString: "this is deprecated"
postgres:
host: localhost
port: 5432
`,
},
}

executeDeprecationTestCase(t, testCases)
}

func executeDeprecationTestCase(t *testing.T, testCases []deprecatedOptionTest) {
for _, testCase := range testCases {
testCase.fileContent = strings.ReplaceAll(testCase.fileContent, "\t", "")

fileName := "tracetest.yaml"
err := ioutil.WriteFile(fileName, []byte(testCase.fileContent), 0644)
require.NoError(t, err)
defer os.Remove(fileName)

logger := logger{
messages: []string{},
}

config.New(nil, &logger)

warnings := make([]string, 0)
for _, line := range logger.GetMessages() {
if line == "" {
continue
}

warnings = append(warnings, line)
}

assert.Len(t, warnings, len(testCase.expectedDeprecatedKeys))
for _, message := range testCase.expectedDeprecatedKeys {
assertSlicesContainsString(t, warnings, message)
}
}
}

func assertSlicesContainsString(t *testing.T, slice []string, str string) {
for _, item := range slice {
if strings.Contains(item, str) {
return
}
}

assert.Fail(t, fmt.Sprintf("%s was not found", str))
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This test is a bit too complex, and the approach of persisting configs to a file on runtime is prone to race conditions and other kinds of errors.

Given that the config options are defined once for all the souces (file, env, flags), you can leverage the flags to allow concurrent tests. This also allows to reuse the testdata/basic.yaml test file, so we don't have to maintain another basic example here.

Here's an example of a simpler implementation of this test. It also verifies the complete output messages, so the coverage is more complete.

Suggested change
type deprecatedOptionTest struct {
name string
fileContent string
expectedDeprecatedKeys []string
}
type logger struct {
messages []string
}
func (l *logger) Println(in ...any) {
message := fmt.Sprintf("%s", in...)
l.messages = append(l.messages, message)
}
func (l *logger) GetMessages() []string {
return l.messages
}
func TestDeprecatedOptions(t *testing.T) {
testCases := []deprecatedOptionTest{
{
name: "should_not_detect_any_deprecated_config",
expectedDeprecatedKeys: []string{},
fileContent: `
postgres:
host: localhost
port: 5432
`,
},
{
name: "should_detect_any_deprecated_config",
expectedDeprecatedKeys: []string{"postgresConnString"},
fileContent: `
postgresConnString: "this is deprecated"
postgres:
host: localhost
port: 5432
`,
},
}
executeDeprecationTestCase(t, testCases)
}
func executeDeprecationTestCase(t *testing.T, testCases []deprecatedOptionTest) {
for _, testCase := range testCases {
testCase.fileContent = strings.ReplaceAll(testCase.fileContent, "\t", "")
fileName := "tracetest.yaml"
err := ioutil.WriteFile(fileName, []byte(testCase.fileContent), 0644)
require.NoError(t, err)
defer os.Remove(fileName)
logger := logger{
messages: []string{},
}
config.New(nil, &logger)
warnings := make([]string, 0)
for _, line := range logger.GetMessages() {
if line == "" {
continue
}
warnings = append(warnings, line)
}
assert.Len(t, warnings, len(testCase.expectedDeprecatedKeys))
for _, message := range testCase.expectedDeprecatedKeys {
assertSlicesContainsString(t, warnings, message)
}
}
}
func assertSlicesContainsString(t *testing.T, slice []string, str string) {
for _, item := range slice {
if strings.Contains(item, str) {
return
}
}
assert.Fail(t, fmt.Sprintf("%s was not found", str))
}
import (
"strings"
"testing"
"github.com/kubeshop/tracetest/server/config"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
type logger struct {
messages []string
}
func (l *logger) Println(in ...any) {
l.messages = append(l.messages, strings.Join(in))
}
func TestDeprecatedOptions(t *testing.T) {
testCases := []struct {
name string
flags []string
expectedMessages []string
}{
{
name: "NoDeprecations",
flags: []string{"--config", "./testdata/basic.yaml"},
expectedMessages: []string{""},
},
{
name: "Deprecated/postgresConnString",
flags: []string{"--postgresConnString", "some conn string"},
expectedMessages: []string{`config "postgresConnString" is deprecated. Use the new postgres config structure instead`},
},
}
configFromFlagsWithLogger := func(logger *logger, inputFlags []string) *config.Config {
flags := pflag.NewFlagSet("fake", pflag.ExitOnError)
config.SetupFlags(flags)
err := flags.Parse(inputFlags)
require.NoError(t, err)
cfg, err := config.New(flags, logger)
require.NoError(err)
return cfg
}
for _, testCase := range testCases {
t.Run(testCase.name, func(t *testing.T) {
testCase := testCase
t.Parallel()
logger := &logger{}
cfg := configFromFlagsWithLogger(testCase.flags)
assert.DeepEqual(t, testCase.expectedMessages, logger.messages)
})
}
}

Copy link
Collaborator

@schoren schoren left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for dealing with my annoying request!

@mathnogueira mathnogueira merged commit b7f9f9a into new-config Feb 23, 2023
@mathnogueira mathnogueira deleted the feat/config-deprecation branch February 23, 2023 19:29
mathnogueira added a commit that referenced this pull request Feb 23, 2023
* update options to use named fields

* add postgresConnString as deprecated

* remove nil validations

* fix deprecated config fields detection

* add logger to config New function

* replace zap with go's log package

* invert logic of printing the deprecation note

Co-authored-by: Sebastian Choren <schoren@users.noreply.github.com>

* unexport logger interface

* use camelCase names in tests

* update test with Sebastian notes and update logic to make it pass

---------

Co-authored-by: Sebastian Choren <schoren@users.noreply.github.com>
schoren added a commit that referenced this pull request Feb 28, 2023
* update options to use named fields

* add postgresConnString as deprecated

* remove nil validations

* fix deprecated config fields detection

* add logger to config New function

* replace zap with go's log package

* invert logic of printing the deprecation note

Co-authored-by: Sebastian Choren <schoren@users.noreply.github.com>

* unexport logger interface

* use camelCase names in tests

* update test with Sebastian notes and update logic to make it pass

---------

Co-authored-by: Sebastian Choren <schoren@users.noreply.github.com>
schoren added a commit that referenced this pull request Mar 1, 2023
* update options to use named fields

* add postgresConnString as deprecated

* remove nil validations

* fix deprecated config fields detection

* add logger to config New function

* replace zap with go's log package

* invert logic of printing the deprecation note

Co-authored-by: Sebastian Choren <schoren@users.noreply.github.com>

* unexport logger interface

* use camelCase names in tests

* update test with Sebastian notes and update logic to make it pass

---------

Co-authored-by: Sebastian Choren <schoren@users.noreply.github.com>
xoscar added a commit that referenced this pull request Mar 1, 2023
* feat(server): implement viper (#1977)

* config(server): allow provisioning from file or environent variable (#1999)

* chore: disable application exporter in demo (#2012)

disable app exporter in demo

* feature(examples): adding aws terraform example

* updating the tracetest image

* feat(server): implement viper (#1977)

* config(server): allow provisioning from file or environent variable (#1999)

* feat: config deprecation (#2015)

* update options to use named fields

* add postgresConnString as deprecated

* remove nil validations

* fix deprecated config fields detection

* add logger to config New function

* replace zap with go's log package

* invert logic of printing the deprecation note

Co-authored-by: Sebastian Choren <schoren@users.noreply.github.com>

* unexport logger interface

* use camelCase names in tests

* update test with Sebastian notes and update logic to make it pass

---------

Co-authored-by: Sebastian Choren <schoren@users.noreply.github.com>

* fix: postgres config retrocompatibility  (#2043)

feat: ensure retrocompatibility if users is using postgresConnString

* feature: adding support for AWS X-RAY (#1987)

* feature: adding support for AWS X-RAY

* updating README

* cleanup

* feat(server): implement viper (#1977)

* added quick-start golang app in example (#1940)

* added quick-start golnag app in example

* Added quick-start-python in examples

* chore(docs): add recipe for datadog (#1995)

* chore(examples): adding documentation and example of datadog integration

* adding datadog recipe

* fixing details in docs

* Apply suggestions from code review

Co-authored-by: Julianne Fermi <julianne@kubeshop.io>

---------

Co-authored-by: Julianne Fermi <julianne@kubeshop.io>

* Update docs structure and config guides (#1984)

* docs(1919): flatten tools and integration

* docs(1919): flatten examples and tutorials

* docs(recipes): edit naming

* docs(config): fix 1982

* examples(tempo): edit config

* docs(structure): add redirect

* docs(config): update dd

* feature(examples): adding net core x tracetest example (#1978)

* feature(examples): adding net core x tracetest example

* adding readme

* addressing feedback comments

* adding more information

* adding support for the native xray client

* refactor connectivity test to reuse components (#1994)

* refactor connectivity test to reuse components

* only run next step if previous step succeeded

* simplify tester TestConnection

Co-authored-by: Sebastian Choren <schoren@users.noreply.github.com>

* add protocol in IsReachable function

* ignore unused ctx

* remove boolean return from IsReachable

* use multierrors to wrap all endpoint errors

* fix build

* use const for IsReachable timeout

* rename IsReachable to CheckReachability

---------

Co-authored-by: Sebastian Choren <schoren@users.noreply.github.com>

* cleanup

* using the new test connection logic

* config(server): allow provisioning from file or environent variable (#1999)

* cleanup

* adding support for session token

* feat(server): implement viper (#1977)

* config(server): allow provisioning from file or environent variable (#1999)

* moving trace id generation to data store

* fixing unit tests

* chore: disable application exporter in demo (#2012)

disable app exporter in demo

* removing examples

* cleanup

---------

Co-authored-by: Sebastian Choren <schoren@users.noreply.github.com>
Co-authored-by: Mahesh Kasbe <60398112+maheshkasabe@users.noreply.github.com>
Co-authored-by: Daniel Baptista Dias <danielbdias@users.noreply.github.com>
Co-authored-by: Julianne Fermi <julianne@kubeshop.io>
Co-authored-by: Adnan Rahić <adnan@kubeshop.io>
Co-authored-by: Matheus Nogueira <matheus.nogueira2008@gmail.com>

* feature(examples) adding tracetest x aws-xray examples (#2030)

* feature: adding support for AWS X-RAY

* updating README

* cleanup

* feat(server): implement viper (#1977)

* added quick-start golang app in example (#1940)

* added quick-start golnag app in example

* Added quick-start-python in examples

* chore(docs): add recipe for datadog (#1995)

* chore(examples): adding documentation and example of datadog integration

* adding datadog recipe

* fixing details in docs

* Apply suggestions from code review

Co-authored-by: Julianne Fermi <julianne@kubeshop.io>

---------

Co-authored-by: Julianne Fermi <julianne@kubeshop.io>

* Update docs structure and config guides (#1984)

* docs(1919): flatten tools and integration

* docs(1919): flatten examples and tutorials

* docs(recipes): edit naming

* docs(config): fix 1982

* examples(tempo): edit config

* docs(structure): add redirect

* docs(config): update dd

* feature(examples): adding net core x tracetest example (#1978)

* feature(examples): adding net core x tracetest example

* adding readme

* addressing feedback comments

* adding more information

* adding support for the native xray client

* refactor connectivity test to reuse components (#1994)

* refactor connectivity test to reuse components

* only run next step if previous step succeeded

* simplify tester TestConnection

Co-authored-by: Sebastian Choren <schoren@users.noreply.github.com>

* add protocol in IsReachable function

* ignore unused ctx

* remove boolean return from IsReachable

* use multierrors to wrap all endpoint errors

* fix build

* use const for IsReachable timeout

* rename IsReachable to CheckReachability

---------

Co-authored-by: Sebastian Choren <schoren@users.noreply.github.com>

* cleanup

* using the new test connection logic

* config(server): allow provisioning from file or environent variable (#1999)

* cleanup

* adding support for session token

* feat(server): implement viper (#1977)

* config(server): allow provisioning from file or environent variable (#1999)

* moving trace id generation to data store

* fixing unit tests

* chore: disable application exporter in demo (#2012)

disable app exporter in demo

* removing examples

* feature(examples) adding tracetest x aws-xray examples

* cleanup

* cleanup

* removing mysql dependency

---------

Co-authored-by: Sebastian Choren <schoren@users.noreply.github.com>
Co-authored-by: Mahesh Kasbe <60398112+maheshkasabe@users.noreply.github.com>
Co-authored-by: Daniel Baptista Dias <danielbdias@users.noreply.github.com>
Co-authored-by: Julianne Fermi <julianne@kubeshop.io>
Co-authored-by: Adnan Rahić <adnan@kubeshop.io>
Co-authored-by: Matheus Nogueira <matheus.nogueira2008@gmail.com>

* cleanup

---------

Co-authored-by: Sebastian Choren <schoren@users.noreply.github.com>
Co-authored-by: Matheus Nogueira <matheus.nogueira2008@gmail.com>
Co-authored-by: Mahesh Kasbe <60398112+maheshkasabe@users.noreply.github.com>
Co-authored-by: Daniel Baptista Dias <danielbdias@users.noreply.github.com>
Co-authored-by: Julianne Fermi <julianne@kubeshop.io>
Co-authored-by: Adnan Rahić <adnan@kubeshop.io>
schoren added a commit that referenced this pull request Mar 2, 2023
* update options to use named fields

* add postgresConnString as deprecated

* remove nil validations

* fix deprecated config fields detection

* add logger to config New function

* replace zap with go's log package

* invert logic of printing the deprecation note

Co-authored-by: Sebastian Choren <schoren@users.noreply.github.com>

* unexport logger interface

* use camelCase names in tests

* update test with Sebastian notes and update logic to make it pass

---------

Co-authored-by: Sebastian Choren <schoren@users.noreply.github.com>
schoren added a commit that referenced this pull request Mar 3, 2023
* update options to use named fields

* add postgresConnString as deprecated

* remove nil validations

* fix deprecated config fields detection

* add logger to config New function

* replace zap with go's log package

* invert logic of printing the deprecation note

Co-authored-by: Sebastian Choren <schoren@users.noreply.github.com>

* unexport logger interface

* use camelCase names in tests

* update test with Sebastian notes and update logic to make it pass

---------

Co-authored-by: Sebastian Choren <schoren@users.noreply.github.com>
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

3 participants