Skip to content

feat: allow setting configuration arguments with environment variables#142

Open
TheSabari07 wants to merge 1 commit into
OpenCHAMI:mainfrom
TheSabari07:fix/issue-21-env-vars
Open

feat: allow setting configuration arguments with environment variables#142
TheSabari07 wants to merge 1 commit into
OpenCHAMI:mainfrom
TheSabari07:fix/issue-21-env-vars

Conversation

@TheSabari07

Copy link
Copy Markdown

Checklist

  • My code follows the style guidelines of this project
  • I have added/updated comments where needed
  • I have added tests that prove my fix is effective or my feature works
  • I have run make test (or equivalent) locally and all tests pass
  • DCO Sign-off: All commits are signed off (git commit -s) with my real name and email
  • REUSE Compliance:
    • Each new/modified source file has SPDX copyright and license headers
    • Any non-commentable files include a <filename>.license sidecar
    • All referenced licenses are present in the LICENSES/ directory

Description

This PR resolves the issue where POSIX-invalid environment variables were being expected by Viper for flags that contained hyphens (e.g. --log-level) or dots (e.g. scan.ports).

Changes Include:

  1. Added viper.SetEnvKeyReplacer to cmd/root.go to globally map hyphenated and dotted flags to their uppercase underscore equivalents.
  2. Updated README.md to include a full, comprehensive table of all available configuration environment variables.
  3. Updated the main manual page (man/magellan.1.sc) to include a new # ENVIRONMENT VARIABLES section to match the README.md documentation.

Fixes #21

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update

For more info, see Contributing Guidelines.

@TheSabari07
TheSabari07 force-pushed the fix/issue-21-env-vars branch from c9d8c44 to 2fd4eb6 Compare July 23, 2026 05:26
@davidallendj

Copy link
Copy Markdown
Collaborator

Just took a quick glance and it looks good so far. I'm going to try using the env vars and do a quick review in a bit.

@davidallendj

davidallendj commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator

It looks like SCAN_INSECURE is missing for magellan scan --insecure which it looks like we have for the other commands.

Flags:
      --disable-cache        Disable saving found assets to a cache database specified with 'cache' flag
      --disable-probing      Disable probing found assets for Redfish service(s) running on BMC nodes
  -F, --format DataFormat    Output format (json, yaml)
  -h, --help                 help for scan
      --include strings      Asset types to scan for (bmcs, pdus) (default [bmcs])
  -i, --insecure             Skip TLS certificate verification during probe
  -o, --output string        Output file path (for json/yaml formats)
      --port ints            Adds additional ports to scan for each host with unspecified ports.
      --protocol string      Set the default protocol to use in scan. (default "tcp")
      --scheme string        Set the default scheme to use if not specified in host URI. (default "https")
      --subnet strings       Add additional hosts from specified subnets to scan.
      --subnet-mask ipMask   Set the default subnet mask to use for with all subnets not using CIDR notation. (default ffffff00)

I think we would also want to change INSECURE -> COLLECT_INSECURE for collect.

Comment thread man/magellan.1.sc

# ENVIRONMENT VARIABLES

The *magellan* CLI tool allows configuring its flags using environment variables. These are automatically parsed based on the flag names, with dots (".") and hyphens ("-") converted to underscores ("_"), and fully capitalized.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Could you mention that multi-argument environment variables are delimited using a comma (or whatever the delimiter is here)?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I've added a note to both the README.md and the man pages clarifying that multi-argument variables should be separated by commas.

@davidallendj

davidallendj commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator

I tried these two environment variables and was not able to do a scan.

$ SCAN_SUBNET=172.18.0.0/24 SCAN_PORTS=5000 ./magellan scan -l info -i
{"level":"error","time":"2026-07-23T09:44:34-06:00","caller":"/home/allend/Work/openchami/magellan/cmd/scan.go:91","message":"nothing to do (no valid target hosts)"}
$ export SCAN_SUBNET=172.18.0.0/24                         
$ export SCAN_PORTS=5000
$ ./magellan scan -l info -i
{"level":"error","time":"2026-07-23T09:45:10-06:00","caller":"/home/allend/Work/openchami/magellan/cmd/scan.go:91","message":"nothing to do (no valid target hosts)"}

As a reference, this would be the equivalent command with flags with JSON output:

$ ./magellan scan --subnet 172.18.0.0/24 -l info -i --port 5000 -F json 
[
  {
    "host": "https://172.18.0.1",
    "port": 5000,
    "protocol": "tcp",
    "state": true,
    "timestamp": "2026-07-23T09:47:33.726986171-06:00",
    "service_type": "Redfish"
  },
  {
    "host": "https://172.18.0.2",
    "port": 5000,
    "protocol": "tcp",
    "state": true,
    "timestamp": "2026-07-23T09:47:33.727042948-06:00",
    "service_type": "Redfish"
  }
]

Resolves Issue OpenCHAMI#21.
Added viper.SetEnvKeyReplacer to map hyphenated and dotted flags to their
uppercase underscore equivalents (e.g., --log-level -> LOG_LEVEL).
Updated README.md and man pages to document the available variables.

Signed-off-by: Sabari07 <sabursd18@gmail.com>
@TheSabari07
TheSabari07 force-pushed the fix/issue-21-env-vars branch from 2fd4eb6 to b6522e6 Compare July 25, 2026 05:20
@TheSabari07

Copy link
Copy Markdown
Author

It looks like SCAN_INSECURE is missing for magellan scan --insecure which it looks like we have for the other commands.

Flags:
      --disable-cache        Disable saving found assets to a cache database specified with 'cache' flag
      --disable-probing      Disable probing found assets for Redfish service(s) running on BMC nodes
  -F, --format DataFormat    Output format (json, yaml)
  -h, --help                 help for scan
      --include strings      Asset types to scan for (bmcs, pdus) (default [bmcs])
  -i, --insecure             Skip TLS certificate verification during probe
  -o, --output string        Output file path (for json/yaml formats)
      --port ints            Adds additional ports to scan for each host with unspecified ports.
      --protocol string      Set the default protocol to use in scan. (default "tcp")
      --scheme string        Set the default scheme to use if not specified in host URI. (default "https")
      --subnet strings       Add additional hosts from specified subnets to scan.
      --subnet-mask ipMask   Set the default subnet mask to use for with all subnets not using CIDR notation. (default ffffff00)

I think we would also want to change INSECURE -> COLLECT_INSECURE for collect.

Thanks for catching this,
I've added the missing bindings for SCAN_INSECURE and COLLECT_INSECURE in the code, and updated the docs to match

@TheSabari07

TheSabari07 commented Jul 25, 2026

Copy link
Copy Markdown
Author

I tried these two environment variables and was not able to do a scan.

$ SCAN_SUBNET=172.18.0.0/24 SCAN_PORTS=5000 ./magellan scan -l info -i
{"level":"error","time":"2026-07-23T09:44:34-06:00","caller":"/home/allend/Work/openchami/magellan/cmd/scan.go:91","message":"nothing to do (no valid target hosts)"}
$ export SCAN_SUBNET=172.18.0.0/24                         
$ export SCAN_PORTS=5000
$ ./magellan scan -l info -i
{"level":"error","time":"2026-07-23T09:45:10-06:00","caller":"/home/allend/Work/openchami/magellan/cmd/scan.go:91","message":"nothing to do (no valid target hosts)"}

As a reference, this would be the equivalent command with flags with JSON output:

$ ./magellan scan --subnet 172.18.0.0/24 -l info -i --port 5000 -F json 
[
  {
    "host": "https://172.18.0.1",
    "port": 5000,
    "protocol": "tcp",
    "state": true,
    "timestamp": "2026-07-23T09:47:33.726986171-06:00",
    "service_type": "Redfish"
  },
  {
    "host": "https://172.18.0.2",
    "port": 5000,
    "protocol": "tcp",
    "state": true,
    "timestamp": "2026-07-23T09:47:33.727042948-06:00",
    "service_type": "Redfish"
  }
]

Hi David! I investigated why the environment variables didn't work for you, and it looks like there's a deeper architectural issue in how the CLI handles configurations.

While the new SetEnvKeyReplacer correctly allows Viper to read environment variables (like SCAN_SUBNETS), the CLI commands never actually read these values back from Viper Because the commands rely strictly on Cobra's flag variables (which don't automatically sync with Viper), the environment variables are currently being ignored across the entire CLI.

How would you like to handle this? Should we add a PersistentPreRun hook in root.go to globally sync Viper's values back to the Cobra flags, or should I open a separate issue to track this refactor?

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.

[DEV] Allow setting certain arguments with environment variables

2 participants