Skip to content

Fix TestDockerPluginSuite/TestPluginInstallArgs nil panic#41180

Open
yedamao wants to merge 1 commit intomoby:masterfrom
yedamao:fix-testing-panic
Open

Fix TestDockerPluginSuite/TestPluginInstallArgs nil panic#41180
yedamao wants to merge 1 commit intomoby:masterfrom
yedamao:fix-testing-panic

Conversation

@yedamao
Copy link
Contributor

@yedamao yedamao commented Jul 6, 2020

fix #40932 the embedding *types.PluginConfig is nil

Signed-off-by: HuanHuan Ye logindaveye@gmail.com

- What I did
Fix TestDockerPluginSuite/TestPluginInstallArgs panic

- How I did it
fix the embedding *types.PluginConfig is nil

- How to verify it
go test -v --run=TestDockerPluginSuite

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

fix the embedding *types.PluginConfig is nil

Signed-off-by: HuanHuan Ye <logindaveye@gmail.com>
}

var cfg Config
cfg.PluginConfig = &types.PluginConfig{}
Copy link
Member

Choose a reason for hiding this comment

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

Thinking what would be better; set PluginConfig here, or update TestPluginInstallArgs to take this case into account;

plugin.CreateInRegistry(ctx, pName, nil, func(cfg *plugin.Config) {
cfg.Env = []types.PluginEnv{{Name: "DEBUG", Settable: []string{"value"}}}
})

And change the above to:

	plugin.CreateInRegistry(ctx, pName, nil, func(cfg *plugin.Config) {
		if cfg.PluginConfig == nil {
			cfg.PluginConfig = &types.PluginConfig{}
		}
		cfg.Env = []types.PluginEnv{{Name: "DEBUG", Settable: []string{"value"}}}
	})

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

how about this:

cfg := Config{PluginConfig: &types.PluginConfig{}}

Copy link
Member

Choose a reason for hiding this comment

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

You mean in the plugin.CreateInRegistry(... code or in the CreateInRegistry() fixture?

In the plugin.CreateInRegistry(..., that wouldn't be correct, because it would overwrite other options in the config.

If you meant in the CreateInRegistry() fixture; yes, that would work, but would be roughly the same as the change is now.

My train of thought for my first comment was that (I think) TestPluginInstallArgs should be able to work with a "default" / "empty" config; from that perspective it would be "cleaner" if we didn't modify the fixture, but adjusted the test to handle that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the embedding pointer to PluginConfig should be initialized when cfg Config be declared.

if check and initial cfg.PluginConfig in CreateOpt function, other CreateOpt function (if have) should do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TestDockerPluginSuite/TestPluginInstallArgs nil panic on s390x and ppc64le

2 participants