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

Accept env var PCIIDS_LOCAL_PATH to read a local file instead of downloading from GitHub #3

Merged
merged 1 commit into from
Sep 30, 2022

Conversation

yeahdongcn
Copy link
Contributor

@yeahdongcn yeahdongcn commented Sep 28, 2022

My user case could be described as follows:

  1. My program may run in a network-restricted environment
  2. My program may be bundled inside a docker image and deployed into a Kubernetes cluster. I can mount a particular directory from the host to the container with predefined pci.ids file

Tests:

❯ PCIIDS_LOCAL_PATH=pci.ids ./pciids --debug=true 1ed5
DEBUG looking up 1ed5                              
DEBUG using local path pci.ids                     
DEBUG parsing vendor IDs                           
DEBUG parsing PCI IDs                              
DEBUG found 19 result(s)
❯ ./pciids --debug=true 1ed5
DEBUG looking up 1ed5                              
DEBUG downloading https://raw.githubusercontent.com/pciutils/pciids/master/pci.ids 
DEBUG 200 OK                                       
DEBUG parsing vendor IDs                           
DEBUG parsing PCI IDs                              
DEBUG found 19 result(s)   

Signed-off-by: Xiaodong Ye yeahdongcn@gmail.com

@powersj
Copy link
Owner

powersj commented Sep 28, 2022

hi again!

Instead of having a mix of environment variables and flags, what do you think about doing this as a flag (e.g. --file?

@yeahdongcn
Copy link
Contributor Author

That is doable 🤔

But the side effect is all exported functions need to take an extra parameter. For backward compatibility, this may not be a good idea.

From CLI point of view, adding a flag to horner to the newly added env var should work too.

Copy link
Owner

@powersj powersj left a comment

Choose a reason for hiding this comment

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

Thought about this some more and I actually have come back around and like how you have this :)

Can you change one debug message and then this is good to go. Thanks again!

query.go Outdated Show resolved Hide resolved
…loading from GitHub

Signed-off-by: Xiaodong Ye <xiaodong.ye@mthreads.com>
@yeahdongcn
Copy link
Contributor Author

Could you please tag a new release? Thanks.

@powersj
Copy link
Owner

powersj commented Sep 30, 2022

Already done! v2.4.0 is out

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.

2 participants