Fix Gemfile.lock.release handling for plugins #22075

agrare · 2022-08-24T16:21:46Z

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.

agrare · 2022-08-24T16:53:09Z

lib/manageiq/environment.rb

@@ -16,7 +16,6 @@ def self.manageiq_plugin_update(plugin_root = nil, force_bundle_update: true)
      # determine plugin root dir. Assume we are called from a 'bin/' script in the plugin root
      plugin_root ||= Pathname.new(caller_locations.last.absolute_path).dirname.parent

-      setup_gemfile_lock if ENV["CI"]


At this point the Gemfile.lock will have already been patched and this would just overwrite it.

I think we still need this for build...or does build manually copy the file?

The build doesn't call bin/setup in each plugin dir, it only does this for the core repo

Oh ok - I forgot that core doesn't call into these methods...I really need to push my branch where it does, cause I keep confusing myself haha.

Ok. So is this line needed or not?

agrare · 2022-08-24T16:54:23Z

bin/ci/patch_plugin_gemfile_lock

+with open('Gemfile.lock') as f:
+  gemfile_lock = f.read()
+
+gemfile_lock_patched = re.compile("GIT\n  remote: %s\n  revision: .+\n  branch: .+\n" %(os.environ['GITHUB_REPOSITORY'])).sub('PATH\n  remote: .\n', gemfile_lock)


NOTE these GitHub actions env vars don't with with cross-repo but this is no different from the existing before_install checks so I left it for now

Do we need to log the Gemfile.lock before and after patching? Or maybe just the diff? I was hoping to see what this looks like in the CI log.

There's a typo in your comment... I'm not sure what you're saying.

Fixed, it was supposed to be env vars, basically when running cross-repo for the oparin branch the GITHUB_BASE_REF is still master because the PR is created in the cross-repo repository. This is used https://github.com/ManageIQ/manageiq/blob/master/bin/before_install#L25 and will be used by the providers, just means we can't test this easily with cross-repo.

ah... shucks... I just asked below if we can test it with cross repo to see how it works. I guess you can do the old fashioned way and just create a WIP plugin PR to use this code to verify it works.

Yeah I just committed a Gemfile.lock.release to bypass the need to copy it from core

Do we need to log the Gemfile.lock before and after patching? Or maybe just the diff? I was hoping to see what this looks like in the CI log.

Gemfile.lock is already logged in CI.

I was more interested in seeing what it changed and less about the full contents.

jrafanie

I'd like to see an end to end run of this in CI to better understand what's being patched but I like the idea of surgically fixing this for release branches to ensure we're testing with the same gems.

bin/ci/patch_plugin_gemfile_lock

jrafanie · 2022-08-24T18:42:31Z

bin/ci/patch_plugin_gemfile_lock

+with open('Gemfile.lock') as f:
+  gemfile_lock = f.read()
+
+gemfile_lock_patched = re.compile("GIT\n  remote: %s\n  revision: .+\n  branch: .+\n" %(os.environ['GITHUB_REPOSITORY'])).sub('PATH\n  remote: .\n', gemfile_lock)


Do we need to log the Gemfile.lock before and after patching? Or maybe just the diff? I was hoping to see what this looks like in the CI log.

jrafanie · 2022-08-24T18:43:25Z

bin/ci/patch_plugin_gemfile_lock

+with open('Gemfile.lock') as f:
+  gemfile_lock = f.read()
+
+gemfile_lock_patched = re.compile("GIT\n  remote: %s\n  revision: .+\n  branch: .+\n" %(os.environ['GITHUB_REPOSITORY'])).sub('PATH\n  remote: .\n', gemfile_lock)


There's a typo in your comment... I'm not sure what you're saying.

agrare · 2022-08-24T18:59:34Z

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)

bdunne · 2022-08-24T22:42:23Z

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)

Will the bundle install in the ruby setup update any dependencies under specs: if they have changed in the plugin gemspec?

agrare · 2022-08-25T13:22:51Z

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

agrare · 2022-10-03T14:48:20Z

@Fryguy what do you think about this?

miq-bot · 2023-02-27T00:03:48Z

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.

kbrock · 2023-05-24T18:05:21Z

@Fryguy you good to merge?

kbrock · 2023-05-31T00:19:09Z

Is this good to go?

Fryguy · 2023-06-05T14:02:44Z

Honestly I don't remember - this was a cross-repo thing, but I'm not sure it's needed?

agrare · 2023-06-05T14:06:54Z

@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)

Fryguy · 2023-06-05T15:00:24Z

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])

agrare · 2023-06-05T15:49:03Z

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

Fryguy · 2023-07-21T22:08:34Z

I thought setup_gemfile_lock was also called in core bin/setup, so specs would use it. But I don't see that here. 🤔

kbrock · 2023-07-21T23:42:53Z

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

kbrock · 2023-09-28T01:25:21Z

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 👍 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?

agrare · 2023-10-11T12:22:58Z

I haven't had time to rewrite the script in ruby and retest this whole thing. Are we 👎 on a python script? Is that a blocker for merge?

Fryguy · 2023-10-11T18:43:41Z

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.

kbrock · 2023-10-16T18:12:51Z

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/...

Fryguy · 2023-10-16T18:38:28Z

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?

agrare · 2023-10-16T19:07:59Z

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.

kbrock · 2023-10-20T17:01:58Z

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?

Fryguy · 2023-10-20T18:24:54Z

@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 (

manageiq/bin/before_install

Lines 40 to 45 in 34f0a74

    
           # Gemfile.lock.release only applies to non-master branches and PRs to non-master branches 
        
           if [[ "$GITHUB_REPOSITORY_OWNER" = "ManageIQ" && "$GITHUB_BASE_REF" != "master" && "$GITHUB_REF_NAME" != "master" && "$GITHUB_REF_NAME" != "dependabot/"* ]]; then 
        
             echo "== Setup Gemfile.lock.release ==" 
        
             cp -f "$APP_ROOT/Gemfile.lock.release" "$APP_ROOT/Gemfile.lock" 
        
             echo 
        
           fi

) somehow in the plugin repos?

miq-bot · 2024-01-22T00:00:05Z

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).

miq-bot · 2024-03-11T18:30:35Z

Checked commits agrare/manageiq@508107c~...206b325 with ruby 2.7.8, rubocop 1.56.3, haml-lint 0.51.0, and yamllint
1 file checked, 0 offenses detected
Everything looks fine. 👍

kbrock · 2024-03-11T22:36:20Z

lib/manageiq/environment.rb

-      setup_gemfile_lock if ci?
+


alternatively, if we want to make a slow transition, we can run this unless we are running build version 2:

setup_gemfile_lock if ci? && ENV["BUILD"] != "v2"

and when we upgrade the github/ci.yaml we can set an env BUILD=v2 when we upgrade the ci.yaml to call bin/ci/patch_plugin_gemfile_lock

miq-bot · 2024-04-26T16:55:20Z

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

miq-bot · 2024-07-29T00:00:03Z

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).

miq-bot · 2024-11-01T01:36:31Z

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).

agrare requested review from jrafanie, bdunne and Fryguy as code owners August 24, 2022 16:21

agrare commented Aug 24, 2022

View reviewed changes

jrafanie reviewed Aug 24, 2022

View reviewed changes

agrare force-pushed the dont_setup_gemfile_lock_plugins branch 2 times, most recently from 23ef482 to 78b3650 Compare August 31, 2022 16:57

Fryguy self-assigned this Aug 31, 2022

Fryguy added bug test labels Aug 31, 2022

agrare force-pushed the dont_setup_gemfile_lock_plugins branch from 78b3650 to 8f51075 Compare September 1, 2022 16:48

miq-bot added the stale label Feb 27, 2023

kbrock self-assigned this May 24, 2023

Fryguy removed the stale label Jun 5, 2023

miq-bot added the unmergeable label Jul 21, 2023

agrare force-pushed the dont_setup_gemfile_lock_plugins branch from 8f51075 to 432e8cd Compare July 21, 2023 17:30

miq-bot removed the unmergeable label Jul 21, 2023

miq-bot added the unmergeable label Jul 26, 2023

agrare force-pushed the dont_setup_gemfile_lock_plugins branch from 432e8cd to 21f8197 Compare October 11, 2023 12:24

miq-bot removed the unmergeable label Oct 11, 2023

miq-bot added the stale label Jan 22, 2024

miq-bot added the unmergeable label Mar 7, 2024

agrare added 2 commits March 11, 2024 14:26

Don't setup Gemfile.lock for plugins

508107c

Add a script to patch a Gemfile.lock for a plugin

206b325

agrare force-pushed the dont_setup_gemfile_lock_plugins branch from 21f8197 to 206b325 Compare March 11, 2024 18:26

kbrock reviewed Mar 11, 2024

View reviewed changes

miq-bot removed the unmergeable label Mar 12, 2024

miq-bot added the unmergeable label Apr 26, 2024

kbrock mentioned this pull request May 23, 2024

Ignore Gemfile.lock.release check on revert branches #23047

Merged

Fix Gemfile.lock.release handling for plugins #22075

Are you sure you want to change the base?

Fix Gemfile.lock.release handling for plugins #22075

Conversation

agrare commented Aug 24, 2022 • edited by kbrock Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agrare Aug 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agrare Aug 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrafanie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agrare commented Aug 24, 2022

bdunne commented Aug 24, 2022

agrare commented Aug 25, 2022 • edited Loading

agrare commented Oct 3, 2022

miq-bot commented Feb 27, 2023

kbrock commented May 24, 2023

kbrock commented May 31, 2023

Fryguy commented Jun 5, 2023

agrare commented Jun 5, 2023 • edited Loading

Fryguy commented Jun 5, 2023

agrare commented Jun 5, 2023 • edited Loading

Fryguy commented Jul 21, 2023

kbrock commented Jul 21, 2023

kbrock commented Sep 28, 2023

agrare commented Oct 11, 2023

Fryguy commented Oct 11, 2023

kbrock commented Oct 16, 2023 • edited Loading

Fryguy commented Oct 16, 2023

agrare commented Oct 16, 2023

kbrock commented Oct 20, 2023

Fryguy commented Oct 20, 2023

miq-bot commented Jan 22, 2024

miq-bot commented Mar 11, 2024

kbrock Mar 11, 2024 • edited Loading

Choose a reason for hiding this comment

miq-bot commented Apr 26, 2024

miq-bot commented Jul 29, 2024

miq-bot commented Nov 1, 2024

agrare commented Aug 24, 2022 •

edited by kbrock

Loading

agrare Aug 24, 2022 •

edited

Loading

agrare Aug 24, 2022 •

edited

Loading

agrare commented Aug 25, 2022 •

edited

Loading

agrare commented Jun 5, 2023 •

edited

Loading

agrare commented Jun 5, 2023 •

edited

Loading

kbrock commented Oct 16, 2023 •

edited

Loading

kbrock Mar 11, 2024 •

edited

Loading