Closed ruda closed 6 months ago
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 76.72%. Comparing base (
75e944b
) to head (c836346
).
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
1. If you use dsc
, then you need to change both executable
and display_name
in config. You can do that through environment variables - discovery-ci is doing that, see cli_override_envvars
in utils/commands.py
. As far as I am aware, there are no problems when you change both. We did consider automatically deriving display_name
from executable
, but that was never implemented.
2. display_name
was added in #396 . At the time we changed qpc to not display certain information unless -v
flag is passed, which caused a number of failures in Camayoc. There was a flurry of PRs: #396, #394, #403, #397. #396 was added with the intent of changing executable
to qpc -v
(which I believe we did for few weeks), and was left around because it might be useful for toolbox-based CLI. We eventually abandoned toolbox and moved to RPMs.
3. Which brings us to the current state - do we still need to separate executable
and display_name
? I'm not sure. I guess it may be useful, although I don't think we really benefited from that in last 10 months or so. In CI, you would have qpc/dsc installed from RPM, and both settings set to qpc
or dsc
, respectively. Locally, you would probably have qpc installed from git in the same virtual environment, and both settings set to qpc
. I would like to hear if anyone sets up their environment differently and changes these two settings to different values.
If we decide to proceed with that change, my only request would be to leave some notes in commit description - refer to commit 7396c6860ba8d509928fe08673d7db1bec0a26b0 and GitHub PR number #396, at least. I have my doubts, but maybe it will help someone in the future.
Summary of discussion on Slack:
qpc
from venv created by poetry (different venv than Camayoc). Scenarios like that benefit from splitting executable
and display_name
, so my initial assessment is wrong - we still do benefit from the splitdisplay_name
explicitly. Perhaps it can be derived from executable
automatically. A year ago we thought about using qpc
from container and/or toolbox, and automatic derivation would need to support things from qpc
to podman run -ti --rm discovery-image /app/bin/qpc -v
. Today we probably only need to care about executable name (dsc
), executable path (/home/user/.venv/qpc/bin/qpc
) and either with flags.To give more context, I have set in camayoc config. I'm not using the absolute path for dsc
.
quipucords_cli:
executable: "dsc"
When Camayoc assumes qpc
as defaults to quipucords_cli.display_name
, it results in test sources and scans failures (those who uses display_name
).
Folks, to avoid going further with aggressive / big impact changes to the framework, I'm proposing to change just the tests in PR #513 and they will use the CLI executable.This is an intermediate step before changing anything regarding the display_name. I'm open to your opinions.
When I installed the
dsc
CLI from RPM, I've discovered that some tests were failing because they're usingqpc
as the discovery command line name instead ofdsc
. This PR fix this misbehavior.