Skip to content

update NetConf.DNS's json tag to omitempty#1007

Closed
cyclinder wants to merge 1 commit intocontainernetworking:mainfrom
cyclinder:dns_omitempty
Closed

update NetConf.DNS's json tag to omitempty#1007
cyclinder wants to merge 1 commit intocontainernetworking:mainfrom
cyclinder:dns_omitempty

Conversation

@cyclinder
Copy link
Copy Markdown

Fixes #1006

@dcbw
Copy link
Copy Markdown
Member

dcbw commented Jul 10, 2023

@cyclinder could you add your Signed-off-by: to the commit? That will fix the DCO check. Thanks!

@coveralls
Copy link
Copy Markdown

Coverage Status

coverage: 72.477%. remained the same when pulling 5241e42 on cyclinder:dns_omitempty into e255525 on containernetworking:main.

@cyclinder cyclinder force-pushed the dns_omitempty branch 6 times, most recently from c4c71ef to 4f3c279 Compare July 11, 2023 02:19
@cyclinder
Copy link
Copy Markdown
Author

Thanks for the tip. Updated.

@cyclinder
Copy link
Copy Markdown
Author

cyclinder commented Jul 11, 2023

Hi @dcbw, I found IPAM and DNS fields are also tagged with omitempty, but this has no effect.

type NetConf struct {
	CNIVersion string `json:"cniVersion,omitempty"`
	IPAM         IPAM            `json:"ipam,omitempty"`
	DNS          DNS             `json:"dns,omitempty"`
}

type IPAM struct {
	Type string `json:"type,omitempty"`
}

type Test struct {
	Name string `json:"name,omitempty"`
}

func main(){
     	t := &test{
	    CNIVersion: "test",
	}

	bytes,err := json.Marshal(t)
	if err != nil {
		log.Fatalln(err)
	}

	log.Println(string(bytes))
}

#cyclinder~ go run main.go
2023/07/11 11:19:00 {"ipam":{},"dns":{},"cniVersion":"test"}

We should make struct to a pointer. Pointers have obvious "empty" values: nil.

type NetConf struct {
	CNIVersion string `json:"cniVersion,omitempty"`
	IPAM         *IPAM            `json:"ipam,omitempty"`
	DNS          *DNS             `json:"dns,omitempty"`
}

type IPAM struct {
	Type string `json:"type,omitempty"`
}

type Test struct {
	Name string `json:"name,omitempty"`
}

func main(){
     	t := &test{
	    CNIVersion: "test",
	}

	bytes,err := json.Marshal(t)
	if err != nil {
		log.Fatalln(err)
	}

	log.Println(string(bytes))
}

#cyclinder~ go run main.go
2023/07/11 11:21:00 {"cniVersion":"test"}

Can we set IPAM and DNS as pointers?

@cyclinder
Copy link
Copy Markdown
Author

@dcbw Hi, Could you take a look? Can we set IPAM and DNS as pointers?

Signed-off-by: cyclinder <qifeng.guo@daocloud.io>
@cyclinder
Copy link
Copy Markdown
Author

Hey @squeed, Could you please take a look? Thanks.

@s1061123
Copy link
Copy Markdown
Contributor

I suppose we also need to change DNS in Result (at least 1.0.0) to pointer as well as NetConf if we change NetConf's DNS type. What do you think about that?

@s1061123
Copy link
Copy Markdown
Contributor

I will cherry-pick your change and create another PR to care about both (NetConf + Result). Please let me know if you mind it. Thanks!

@s1061123
Copy link
Copy Markdown
Contributor

Files #1035 with your commit (as cherry-pick).

@cyclinder
Copy link
Copy Markdown
Author

Sorry for the delay @s1061123 due to busy with working :>

Please let me know if you mind it. Thanks!

It doesn't matter, you are welcome.

@s1061123
Copy link
Copy Markdown
Contributor

Closed because #1039 fixes this issue.

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.

update NetConf.DNS's tag to omitempty

4 participants