[WIP] Add integration Rails environment

[NickLaMuro]

The intent of this is to have a separate environment that can be used for integration tests, and can have specific functionality to it that is propose driven to that environment, instead of trying to shoehorn it into test or development environments.

By default, the environment mirrors production for the most part, but the $CYPRESS_DEV var is designed to be able to toggle portions of the configuration to be more "dev-like" (for example: no compressing assets so TDD can be done while developing integration specs).

There is also included rake tasks to simplify spinning up a server for cypress or other integration style tests to run against and other tasks like seeding the database and compiling assets for the environment.

TODO

Most likely in separate PRs:

• Add "integration only" routes for factory_bot, "database cleaning", and seeding
• Login helpers

Links

• Previous and related efforts: [WIP] Add travis for running cypress integration tests manageiq-ui-classic#6683
• Assisting with the "cypress travis" effort: [WIP] Add travis for running cypress integration tests manageiq-ui-classic#6683
• "pre-fixup" branch (for some context): master...NickLaMuro:integration_env_wip

[NickLaMuro]

@miq-bot add-reviewer @jvlcek
@miq-bot add-reviewer @himdel
@miq-bot assign @jrafanie

[NickLaMuro]

Gemfile

• ❗ - Line 267, Col 23 - Layout/ExtraSpacing - Unnecessary spacing detected.

Invalid rule for the Gemfile. We do this all over the place to align elements of the Hash. This file should probably be an exception in our config for this rule.

lib/tasks/integration.rake

• ⚠️ - Line 61, Col 87 - Lint/Void - Operator == used in void context.

Unsure how I want to handle this yet, so this one is valid (probably the only one). Mostly trying to decide if any form of a response is good enough for the ping, of if we do want to validate if the body is "pong" (if it is not, then we probably need to raise something else and rescue that error as well.)

• ❗ - Line 15, Col 5 - Rails/SkipsModelValidations - Avoid using touch because it skips validations.

Incorrect catch here. Using "touch" from FileUtils, not from Rails.

• ❗ - Line 22, Col 16 - Style/RescueModifier - Avoid using rescue in its modifier form.

Not sure if I agree with this one. Just makes the code simpler to digest.

• ❗ - Line 27, Col 23 - Layout/AlignArray - Align the elements of an array literal if they span more than one line.
• ❗ - Line 28, Col 23 - Layout/AlignArray - Align the elements of an array literal if they span more than one line.

While a "valid catch", I am using this to line up command line arguments, so I think it is better in this form, and more readable.

[Fryguy]

Only concern I have is there are a number of places in the code where we check the Rails environment (and probably in a number of ways too (e.g. instead of Rails.env.production? maybe we do Rails.env == "production" or ENV['RAILS_ENV'] == 'production'). We probably need to audit all of those to see ifeach of those checks needs to also include intergration or not, particularly because this is a sort-of hybrid environment.

[Fryguy]

Instead of generating this on the fly, can we just have a Procfile.integration file committed, and then in setup it's just copied to the right place?

[Fryguy]

Also this appends to a file, but the file is empty to start with, so I'm not sure what this is doing?

[NickLaMuro]

Yeah, probably. My thoughts were that eventually we might need a generic worker at a minimum to test certain things in the app, and so I figured that being able to make this configurable for a local user might be ideal.

But, again, YAGNI and all that, so I am not opposed to just doing a Procfile.integration for now.

Update: Will look into how to do this later today, but want to push the other simpler changes that I have before diving into re-writing this section.

[NickLaMuro]

"Sorta solved" this with the last couple of commits. I am 50/50 on the last commit, and might still make a few tweaks to handling some of the file dependencies since I realize they might be better served as file/dir tasks instead of in a .setup method.

Regardless, it is still "generated" a bit because I want to make sure the run_single_worker.rb path is calculated and not hard coded, so using ERB for that bit at a minimum.

[Fryguy]

Would it be simpler to just to a HEAD request and check for 200? I guess it's not really much of a difference, but less is more.

[NickLaMuro]

I would still want to hit against the /api/ping route since it doesn't even load our base controller:

https://github.com/ManageIQ/manageiq-api/blob/fdc94421077ecedfc509c413b17a566d07dbebd0/app/controllers/api/ping_controller.rb#L1-L7

And so that makes the request much cheaper, where a HEAD / ends up actually making some database calls. I don't know if you could do a HEAD /api/ping, but seems like it would minimal difference if anything.

[Fryguy]

Sorry, I mean HEAD to /api/ping as opposed to GET /api/ping, but yeah it's going to be a pretty minimal difference. My only thought was if for some reason we change the pong to something else, then this creates an unnecessary coupling, that a HEAD could avoid.

[NickLaMuro]

Fair enough. I can do a quick test to see if we would have to make an addition to that controller to do this. I honestly don't know the answer off hand, or what it would look like for the code in Ruby (I think .get_response is a special method for net/http at least).

[NickLaMuro]

Switched to HEAD in some refactoring commits, and it works. The response.code comes back as a string, which I wasn't expecting, but besides that, it is probably a better approach. Requires setting up a http object ahead of time since there isn't a class method for it, so one extra line of code was required.

[Fryguy]

I'm concerned this will break callers of this that might live outside of this app (like from UI classic), though perhaps this is fixing that?

[NickLaMuro]

... though perhaps this is fixing that?

It is, and we actually use this in other places as well, and I just ripped it off (I used the same app_prefix variable name, so you should be able to grep in lib/tasks to find it).

Effectively, if this doesn't need a "app:" prefix, it will just be blank, and call the default task. But when running from manageiq-ui-classic, these tasks need to have the "app_prefix" for them to work.

That said, I was thinking that it might make sense to have a generic helper function for this, but figured it wasn't worth starting on that in the PR, though I could still be convinced if we think it makes sense.

[Fryguy]

I actually found a way to fix a part of this with Rails itself, but never got around to blasting it out...I'd like to pair on it if you have time at some point. However, I think your prefix code here would still be needed.

[Fryguy]

Overall, LGTM!

[NickLaMuro]

Stepping away from this for a few days, but figured I should leave a quick status:

This has shown to be working in practice here, with the last few commits (with [DONOTMERGE]) that will be broken out into a separate PR effort eventually (ran out of time today). For now, they can be viewed in isolation here:

NickLaMuro/manageiq@integration_env^^...integration_env

Those two commits in detail:

• [DONOTMERGE][WIP] Add ManageIQ::Integration::DatabaseCleaner: Creates a custom strategy for cleaning the database. This is described further in he commit, but effectively it makes use of a copy of the seeded integration database that can be connected to by the integration DB and used after a sweep of TRUNCATE calls to reload tables back to their "seeded" state. Some tables are skipped (miq_servers for example), since they are in direct use the entire time, and dropping their records mid running would most likely cause side effects.
• [POC][WIP][DONOTMERGE] Integration Factories Makes FactoryBot, as well as the above ManageIQ::Integration::DatabaseCleaner available through routes in the application. This is a very small engine that just loads works with the existing ManageIQ::Integration code, as well as the spec/support/factory_bot_helper.rb code to integrate those two pieces. Only loaded in a Rails.env.integration? environment (ideally).

I haven't tests this from cypress yet, though I am basically following the strategies outlined in this post:

https://blog.simplificator.com/2019/10/11/setting-up-cypress-with-rails/

And so integrating it from the UI should be straight forward. We might want a shard .js file for this (similar to the lib/manageiq/integration/cypress_rake.rb tasks that the UI is currently consuming, but that will be something I explore next week.

[miq-bot]

Some comments on commits NickLaMuro/manageiq@af7daeb~...90623d0

lib/manageiq/integration/foreman_manager.rb

• ⚠️ - 142 - Detected puts. Remove all debugging statements.
• ⚠️ - 180 - Detected puts. Remove all debugging statements.
• ⚠️ - 181 - Detected puts. Remove all debugging statements.
• ⚠️ - 182 - Detected puts. Remove all debugging statements.
• ⚠️ - 183 - Detected puts. Remove all debugging statements.
• ⚠️ - 191 - Detected puts. Remove all debugging statements.
• ⚠️ - 192 - Detected puts. Remove all debugging statements.
• ⚠️ - 193 - Detected puts. Remove all debugging statements.
• ⚠️ - 88 - Detected puts. Remove all debugging statements.
• ⚠️ - 90 - Detected puts. Remove all debugging statements.
• ⚠️ - 92 - Detected puts. Remove all debugging statements.

[miq-bot]

Checked commits NickLaMuro/manageiq@af7daeb~...90623d0 with ruby 2.6.3, rubocop 0.69.0, haml-lint 0.28.0, and yamllint
12 files checked, 26 offenses detected

Gemfile

• ❗ - Line 270, Col 23 - Layout/ExtraSpacing - Unnecessary spacing detected.
• ❗ - Line 270, Col 37 - Layout/ExtraSpacing - Unnecessary spacing detected.
• ❗ - Line 272, Col 23 - Layout/ExtraSpacing - Unnecessary spacing detected.

app/models/miq_worker/runner.rb

• ❗ - Line 312, Col 31 - Rails/UnknownEnv - Unknown environment integration?.

config/initializers/session_store.rb

• ❗ - Line 1, Col 66 - Rails/UnknownEnv - Unknown environment integration?.

lib/manageiq/integration.rb

• ❗ - Line 7, Col 1 - Layout/EmptyLinesAroundModuleBody - Extra empty line detected at module body beginning.

lib/manageiq/integration/cypress_rake_task.rb

• ⚠️ - Line 51, Col 73 - Lint/UnusedBlockArgument - Unused block argument - t. If it's necessary, use _ or _t as an argument name to indicate that it won't be used.
• ❗ - Line 11, Col 1 - Layout/EmptyLinesAroundClassBody - Extra empty line detected at class body beginning.
• ❗ - Line 52, Col 13 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
• ❗ - Line 54, Col 16 - Layout/ExtraSpacing - Unnecessary spacing detected.
• ❗ - Line 54, Col 18 - Layout/SpaceAroundOperators - Operator = should be surrounded by a single space.
• ❗ - Line 81, Col 13 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.

lib/manageiq/integration/foreman_manager.rb

• ⚠️ - Line 186, Col 32 - Lint/UselessAssignment - Useless assignment to variable - e.
• ⚠️ - Line 206, Col 11 - Lint/HandleExceptions - Do not suppress exceptions.
• ❗ - Line 142, Col 9 - Rails/Output - Do not write to stdout. Use Rails's logger if you want to log.
• ❗ - Line 180, Col 9 - Rails/Output - Do not write to stdout. Use Rails's logger if you want to log.
• ❗ - Line 182, Col 9 - Rails/Output - Do not write to stdout. Use Rails's logger if you want to log.
• ❗ - Line 191, Col 9 - Rails/Output - Do not write to stdout. Use Rails's logger if you want to log.
• ❗ - Line 204, Col 13 - Layout/EmptyLineAfterGuardClause - Add empty line after guard clause.
• ❗ - Line 205, Col 13 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
• ❗ - Line 27, Col 45 - Style/HashSyntax - Use hash rockets syntax.
• ❗ - Line 66, Col 27 - Layout/AlignArray - Align the elements of an array literal if they span more than one line.
• ❗ - Line 67, Col 27 - Layout/AlignArray - Align the elements of an array literal if they span more than one line.
• ❗ - Line 88, Col 11 - Rails/Output - Do not write to stdout. Use Rails's logger if you want to log.
• ❗ - Line 90, Col 11 - Rails/Output - Do not write to stdout. Use Rails's logger if you want to log.
• ❗ - Line 92, Col 9 - Rails/Output - Do not write to stdout. Use Rails's logger if you want to log.

[miq-bot]

This pull request is not mergeable. Please rebase and repush.

[miq-bot]

This pull request has been automatically closed because it has not been updated for at least 3 months.

Feel free to reopen this pull request if these changes are still valid.

Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation.