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

Fixes #33356 - APP_ROOT defined for tests #797

Closed
wants to merge 1 commit into from

Conversation

lzap
Copy link
Member

@lzap lzap commented Aug 26, 2021

OpenSCAP plugin class requires this to be defined, tests do fail when openscap plugin is enabled.

@lzap
Copy link
Member Author

lzap commented Aug 26, 2021

Context: theforeman/smart_proxy_openscap#86

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Please don't rush this. I've noticed that a lot of plugins are starting to build their own constants for this. In particular, I want to introduce something that in production should point to /var/lib/foreman-proxy. It looks like OpenSCAP needs that, but there are many others. Let's think about that properly.

test/test_helper.rb Outdated Show resolved Hide resolved
@lzap
Copy link
Member Author

lzap commented Aug 26, 2021

Rebased, I cannot wait with this, I am unable to continue on OpenSCAP refactoring due to this. I fully support resolving the common storage problem, but it has nothing to do with this - this constant is simply not available when we load plugins from unit tests without loading full smart proxy main stack.

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Let's start with the background. The APP_ROOT is defined here:

APP_ROOT = "#{__dir__}/.."

This is an issue because you have smart_proxy_openscap in your bundle.

For testing, we have an alternative to smart_proxy_main, namely smart_proxy_for_testing. I wonder if that would be the correct place because it would then also be present in modules that don't use the test_helper here but define their own. However, then the root would be the Smart Proxy root, not the plugin root itself.

For reference:
https://github.com/theforeman/smart_proxy_openscap/blob/1af66df4083c5968f4e6489fed7fb6e4f7eb6c21/test/test_helper.rb#L9-L13

Overall I'd say this isn't so urgent because it only happens if smart_proxy_openscap is in your bundle.

@lzap
Copy link
Member Author

lzap commented Aug 26, 2021

I moved this into smart_proxy_for_testing. It still resolves my problem.

Overall I'd say this isn't so urgent because it only happens if smart_proxy_openscap is in your bundle.

In other words, you don't care about me needing to comment out OpenSCAP plugin everytime I work on smart-proxy core PR. Well, how cool is that.

@ekohl
Copy link
Member

ekohl commented Aug 27, 2021

There are other considerations, like what it means. APP_ROOT should really be the application root, but in the case of smart_proxy_openscap what's needed is a DATA_ROOT.

So putting it in smart_proxy_for_testing is correct from a naming perspective. On the other hand, the way it's used in smart_proxy_openscap is different. It means that if you have a git checkout installed via bundler your data files can end up somewhere in ~/.gem.

Overall I'd say this isn't so urgent because it only happens if smart_proxy_openscap is in your bundle.

In other words, you don't care about me needing to comment out OpenSCAP plugin everytime I work on smart-proxy core PR. Well, how cool is that.

There's other workarounds. For example, use this in your bundler.d file:

unless ENV['SKIP_OPENSCAP']
  gem ...
end

Then in your shell you can export SKIP_OPENSCAP=1.

@lzap
Copy link
Member Author

lzap commented Aug 31, 2021

Can you just re-review the current version? There is nothing wrong with this proposal, APP_ROOT is smart proxy application root being defined in a testing ruby helper file.

@lzap
Copy link
Member Author

lzap commented Sep 8, 2021

Ok, how about this - I created DATA_DIR constant and an empty directory in git named ./data/ which has contents in gitignore. It has a README file with explanation:

[lzap@nuc smart-proxy]$ cat data/README
This directory is reserved for data in development mode. To access it, use
DATA_DIR constant. Plugins should create subdirectories within the directory.

The constant is set to /var/lib/foreman-proxy, if such directory exists.
Otherwise this directory is used. When running tests, DATA_DIR is set to
../tmp/data.

@lzap lzap closed this Feb 19, 2023
@lzap lzap deleted the app-root-tests-33356 branch February 19, 2023 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants