manageiq
manageiq copied to clipboard
ManageIQ
Fix Gemfile.lock.release handling for plugins
When plugins run tests on a release branch the Gemfile.lock.release was copied to Gemfile.lock but since the plugin spec doesn't match (GIT vs PATH) the Gemfile.lock was invalidated and rebuilt with the latest gem versions.
This meant that plugin tests on release branches were not actually testing with the gems from the release.
This change patches the Gemfile.lock to fix the engine spec so the Gemfile.lock isn't invalidated. It will have to be called by the bin/before_install script in each plugin.
NOTE this has to be done prior to ruby setup since that runs bundle install, thus I implemented this in Python as that is available on github actions already. Awk/sed all process one line at a time so rebuilding the entire gem block proved impractical though I'm sure it could be done.
And this is what the diff looks like for the manageiq-providers-autosde plugin:
$ GITHUB_SERVER_URL=https://github.com GITHUB_REPOSITORY=ManageIQ/manageiq-providers-autosde ./bin/ci/patch_plugin_gemfile_lock
$ diff Gemfile.lock Gemfile.lock.release
99,100c99,102
< PATH
< remote: .
---
> GIT
> remote: https://github.com/ManageIQ/manageiq-providers-autosde
> revision: 1939a10c1aa1e58e0a89684c59f16b4c114f363e
> branch: oparin
So the new block looks like this:
PATH
remote: .
specs:
manageiq-providers-autosde (0.1.0)
autosde_openapi_client (~> 1.1.0)
typhoeus (~> 1.4)
And this is what the diff looks like for the manageiq-providers-autosde plugin:
$ GITHUB_SERVER_URL=https://github.com GITHUB_REPOSITORY=ManageIQ/manageiq-providers-autosde ./bin/ci/patch_plugin_gemfile_lock $ diff Gemfile.lock Gemfile.lock.release 99,100c99,102 < PATH < remote: . --- > GIT > remote: https://github.com/ManageIQ/manageiq-providers-autosde > revision: 1939a10c1aa1e58e0a89684c59f16b4c114f363e > branch: oparinSo the new block looks like this:
PATH remote: . specs: manageiq-providers-autosde (0.1.0) autosde_openapi_client (~> 1.1.0) typhoeus (~> 1.4)
Will the bundle install in the ruby setup update any dependencies under specs: if they have changed in the plugin gemspec?
Will the bundle install in the ruby setup update any dependencies under specs: if they have changed in the plugin gemspec?
You mean like if a plugin updated their gemspec but we didn't update the Gemfile.lock? The bundle install will fail since it can't resolve the dependencies until we update Gemfile.lock which is what we want rather than silently succeeding but not using the right gem versions.
Ran a test to show this, I downgraded the resolved autosde_openapi_client to 1.0.54 in the Gemfile.lock where the spec requires ~> 1.1.0: https://github.com/ManageIQ/manageiq-providers-autosde/runs/8016809143?check_suite_focus=true#step:5:49
This pull request has been automatically marked as stale because it has not been updated for at least 3 months.
If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s)
Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation.
Honestly I don't remember - this was a cross-repo thing, but I'm not sure it's needed?
@Fryguy this wasn't cross repo, the issue here is that when running specs on release branches on GHA plugins are not using the Gemfile.lock.release In this case this caused the scheduled CI runs to be green where the tests should have actually failed. (PR description does a better job of explaining the issue)
Ok thanks - I double checked and Ruby is installed by default in GHA, so this could be a ruby script:
See https://github.com/Fryguy/sandbox/actions/runs/5178658485/jobs/9330435149#step:10:18 (installed version is ruby 3.0.2p107 (2021-07-07 revision 0db68f0233) [x86_64-linux-gnu])
Hm must have been added since I originally tested this (has been almost a year I guess so that makes sense). Okay I'm going to have to rewrite and retest this then
I thought setup_gemfile_lock was also called in core bin/setup, so specs would use it. But I don't see that here. 🤔
Yea, I can only find it within this file. I'd feel more comfortable with this merged. The current code (before merge) definitely opens us up to versioning nuances
ok, trying to follow this thread
in development, setup_gemfile_lock is not called. so bin/setup and bin/update are not changed.
in ci, bin/setup => manageiq_plugin_setup => manageiq_plugin_update => setup_gemfile_lock
So bin/before_install will need to be changed to call this new script.
It was moved into that script so the gemfile.lock can be setup before all this ruby processing.
I'm :+1: on this. We just need to update the 35 repos with this change
so this is WIP: rewrite in ruby? It is short enough that maybe keeping in python may be more future proof.
ugh. do we just revisit this every 6-12 months?
I haven't had time to rewrite the script in ruby and retest this whole thing. Are we :-1: on a python script? Is that a blocker for merge?
We were just waiting on a rewrite to Ruby. We can merge now with python and change in a follow up.
Separately we need a change to all of the plugins bin/before_install. However, alternatively we may want to move towards running a shared bin/before_install from core. I had started that work but never finished it.
Think I vote keeping python and moving script to manageiq/bin/{ci/}?patch_plugin_gemfile_lock.
So all subprojects will call this single script.
not sure if it will be tricky to link in the ci.yaml file. Maybe as simple as rspec/manageiq/bin/...
I was about to merge but noticed the python script doesn't modify the Gemfile.lock.release, but instead modifies Gemfile.lock. Not sure I understand how that's supposed to work?
I was about to merge but noticed the python script doesn't modify the Gemfile.lock.release, but instead modifies Gemfile.lock. Not sure I understand how that's supposed to work?
Guess I need a better PR description, the problem I was trying to solve was that plugin specs on release branches weren't running with the gems that were in the Gemfile.lock.release because bundler saw the Gemfile.lock as invalid due to the plugin local path override and would bundle update everything.
This script patches the Gemfile.lock after it was copied from Gemfile.lock.release but before bundle install is run.
ok. did that clear up the confusion. Is this ready to go?
Or at least ready to blast out all child projects to point to this common script?
@agrare ok that part makes sense, but the "copy Gemfile.lock.release to Gemfile.lock" line was removed here https://github.com/ManageIQ/manageiq/pull/22075/files#diff-3407d3d6c1f5822da11eac8ba10639efe824e11d65c8bd19ce034253ea124eb9L85. Or is this relying on the core bin/before_install (https://github.com/ManageIQ/manageiq/blob/34f0a74a240d3b92ecf115d3346725469efc0f46/bin/before_install#L40-L45) somehow in the plugin repos?
This pull request has been automatically marked as stale because it has not been updated for at least 3 months.
If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s).