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

cnitool: deduplicate environment variable definitions #926

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

UiP9AV6Y
Copy link

create a reusable package for environment variable to be used by cnitool and the other library code.

Signed-off-by: Gordon Bleux [email protected]

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.06%) to 71.299% when pulling fb89176 on UiP9AV6Y:feature_dedupe_env into 76aaefb on containernetworking:main.

pkg/env/env.go Outdated Show resolved Hide resolved
pkg/env/env.go Outdated Show resolved Hide resolved
pkg/env/env.go Outdated Show resolved Hide resolved
@squeed
Copy link
Member

squeed commented Nov 14, 2022

This looks basically reasonable, but any of the environment variables that are cnitool-specific should probably not be in pkg. That's just because we don't want to make any public library commitments in that regard.

pkg/skel/skel.go Outdated Show resolved Hide resolved
@UiP9AV6Y
Copy link
Author

i have moved all cnitool specific default values back into
the cmd package to prevent having too many empty defaults
(the default has to be defined by the consumer when using
GetVal())

furthermore i have moved the CNI Args parsing logic into
the library API as this is something covered by the
SPEC


// supported CNI_COMMAND values

CmdAdd = "ADD"
Copy link
Member

Choose a reason for hiding this comment

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

Since pkg/env is supposed to be a tool package, is it better to place this constants on another package, maybe pkg/constants?

create a reusable package for environment variables
to be used by `cnitool` and the other library code.

Signed-off-by: Gordon Bleux <[email protected]>
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.

4 participants