Skip to content

Conversation

@rbtr
Copy link
Collaborator

@rbtr rbtr commented Jul 13, 2021

Reason for Change:

Issue Fixed:

Requirements:

Notes:

@rbtr rbtr force-pushed the rbtr/cni-empty-obj branch from ab6f422 to 3d3e0a2 Compare July 14, 2021 17:59
neaggarwMS
neaggarwMS previously approved these changes Jul 14, 2021
tamilmani1989
tamilmani1989 previously approved these changes Jul 14, 2021
Copy link
Member

@tamilmani1989 tamilmani1989 left a comment

Choose a reason for hiding this comment

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

lgtm

@rbtr rbtr dismissed stale reviews from tamilmani1989 and neaggarwMS via b8b0d9e July 14, 2021 22:10
@rbtr rbtr force-pushed the rbtr/cni-empty-obj branch from 3d3e0a2 to b8b0d9e Compare July 14, 2021 22:10
@rbtr
Copy link
Collaborator Author

rbtr commented Jul 14, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

fi, err := os.Stat(filename)
if err != nil {
if errors.Is(err, os.ErrNotExist) {
return nil
Copy link
Member

Choose a reason for hiding this comment

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

Add a log line?

@tamilmani1989 tamilmani1989 merged commit e330355 into master Jul 14, 2021
// empty file on the host filesystem, crashing older CNI because it doesn't know
// how to handle empty statefiles.
func WriteObjectToCNIStatefile() error {
filename := "/var/run/azure-vnet.json"
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @rbtr 👋 what's the best way I can make this different for OS windows?

I was thinking about making a statefile_windows.go with this same code but different filename, that seems to copy alot however.

Any ideas? I just want to defer to you since you know alot more about go than I do, I can only think about an if statement if the OS is windows, just wondering if that's enough

Copy link
Collaborator Author

@rbtr rbtr Oct 5, 2021

Choose a reason for hiding this comment

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

This was a hack for older CNIs, pre 1.4.7 or so; if you're adding support to Windows now then this might not even be necessary.
If it is necessary, is the only thing that's different on Windows the filepath? That could be injected from an early OS check all the way down in to this function. OR, probably more correctly, this shouldn't be a hardcoded const and should come from env or config (with a default for each OS).

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.

5 participants