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

[WIP]: Fix Unmarshalling when only PFlags or Envs are set #766

Closed
wants to merge 1 commit into from
Closed

[WIP]: Fix Unmarshalling when only PFlags or Envs are set #766

wants to merge 1 commit into from

Conversation

inkychris
Copy link
Contributor

Fixes #764 - Regression introduced by #673

The keyComponents() function was using the Get() method in a way similar to how IsSet() works. Neither of these were returning in the expected way for partial keys when only pflags had been bound.

@inkychris inkychris changed the title Fix Unmarshalling when only PFlags are set [WIP]: Fix Unmarshalling when only PFlags are set Sep 18, 2019
@sagikazarmark
Copy link
Collaborator

sagikazarmark commented Sep 18, 2019

Thanks for the PR!

Checking pflag is not enough IMHO. The same issue will cause Set and SetDefault not working. For that reason I think we will have to revert that PR and implement key delimiter instead.

@inkychris
Copy link
Contributor Author

inkychris commented Sep 18, 2019

I'm not seeing the same behaviour for Set and SetDefault because they create an interface for "foo" which is then found by Get or IsSet. The problem before was that while these were returning true, the pflags, and as I just suspected env, were returning nil for "foo" instead, hence the new modified version of IsSet.

I can add the tests for Set and SetDefault to make sure?

@inkychris inkychris changed the title [WIP]: Fix Unmarshalling when only PFlags are set [WIP]: Fix Unmarshalling when only PFlags or Envs are set Sep 18, 2019
@sagikazarmark
Copy link
Collaborator

sagikazarmark commented Sep 18, 2019

Indeed, you are right. :)

Here is a breaking test though:

func TestUnmarshalOnlyPFlagSet2(t *testing.T) {
	flags := pflag.NewFlagSet("test", pflag.ContinueOnError)
	flags.String("foo.bar", "cobra_flag", "")

	v := New()
	assert.NoError(t, v.BindPFlags(flags))

	v.SetConfigType("yaml")
	v.ReadConfig(bytes.NewBuffer([]byte(`foo.bar: file_content`)))

	config := &struct {
		Foo struct {
			Bar string
		}
	}{}

	assert.NoError(t, v.Unmarshal(config))
	assert.Equal(t, "cobra_flag", config.Foo.Bar)
	t.Log(v.AllSettings())
}

Flags and env vars generally override config entries. This is a tough scenario, because keys are not identical in this case.

@inkychris
Copy link
Contributor Author

Yes, I'd noticed that. This change is at least consistent with pre #673 behviour but I guess if we also want a solution to the config override then reverting #673 and implementing a key delimter setter is the only viable way forward.

@sagikazarmark
Copy link
Collaborator

I'm afraid that any breaking change will cause trouble since this library is widely used. I hope we can clear things up a bit in a v2, but for now, I think reverting and allowing to set the key delimiter is a better option.

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.

PR #673 broke my code. The nested structures don't get the default values correctly
2 participants