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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

cyclinder
Copy link

Fixes #1006

@dcbw
Copy link
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

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
Author

Thanks for the tip. Updated.

@cyclinder
Copy link
Author

cyclinder commented Jul 11, 2023 •
edited
Loading

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
Author

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

@cyclinder
Copy link
Author

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

@s1061123
Copy link
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
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 s1061123 mentioned this pull request Oct 26, 2023
Closed
@s1061123
Copy link
Contributor

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

@cyclinder
Copy link
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 s1061123 mentioned this pull request Oct 31, 2023
Merged
@s1061123
Copy link
Contributor

Closed because #1039 fixes this issue.

@s1061123 s1061123 closed this Feb 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

update NetConf.DNS's tag to omitempty
4 participants
@cyclinder @dcbw @coveralls @s1061123