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

Make acli app:new:from:drupal7 default to the latest AMA recommendations on d.o #1588

Merged
merged 2 commits into from
Sep 1, 2023

Conversation

wimleers
Copy link
Member

@wimleers wimleers commented Sep 1, 2023

Motivation
#1543 added acli app:new:from:drupal7. 👍 Next steps in open sourcing: https://git.drupalcode.org/project/acquia_migrate/-/tree/recommendations is live.

So now let's get it all working together. Unfortunately, this made me hit an edge case in GitLab and/or PHP. I opened #1587 for that.

Ideally, we'd get the command to automatically download the latest, to allow you to omit --recommendations.

That'd be a much better UX than having to specify a long URL manually.

Proposed changes

Defaulting to automatically downloading the latest is a one-line change:

-    $recommendations_location = __DIR__ . '/../../../config/from_d7_recommendations.json';
+    $recommendations_location = "https://git.drupalcode.org/project/acquia_migrate/-/raw/recommendations/recommendations.json";

(plus the bugfix in #1587)

+    // PHP defaults to no user agent. (Drupal.org's) GitLab requires it.
+    // @see https://www.php.net/manual/en/filesystem.configuration.php#ini.user-agent
+    ini_set('user_agent', 'ACLI');
     $recommendations_resource = fopen($recommendations_location, 'r');

☝️ That's literally the whole PR! 😄

Alternatives considered
#1587 is the absolute minimum.

Testing steps
Before:

$ ./bin/acli app:new:from:drupal7 --directory=/tmp/foo  --stored-analysis tests/fixtures/drupal7/training.acquia.com/extensions.json 
🤖 Scanning Drupal 7 site.
👍 Found Drupal 7 site (7.70 to be precise) at <location unknown>, with 96 modules enabled!
🤖 Computing recommendations for this Drupal 7 site…
🥳 Great news: found 9 recommendations that apply to this Drupal 7 site, resulting in a composer.json with:
	- 11 packages
	- 1 patches
	- 4 modules to be installed!
🚀 Generated composer.json and committed to a new git repo.
…

After:

$ ./bin/acli app:new:from:drupal7 --directory=/tmp/foo  --stored-analysis tests/fixtures/drupal7/training.acquia.com/extensions.json 
🤖 Scanning Drupal 7 site.
👍 Found Drupal 7 site (7.70 to be precise) at <location unknown>, with 96 modules enabled!
🤖 Computing recommendations for this Drupal 7 site…
🥳 Great news: found 105 recommendations that apply to this Drupal 7 site, resulting in a composer.json with:
	- 46 packages
	- 1 patches
	- 64 modules to be installed!
🚀 Generated composer.json and committed to a new git repo.
…

Note: 11 → 46 packages, 4 → 64 modules!

@codecov
Copy link

codecov bot commented Sep 1, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (4affeb3) 91.76% compared to head (0307191) 91.76%.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #1588   +/-   ##
=========================================
  Coverage     91.76%   91.76%           
  Complexity     1809     1809           
=========================================
  Files           124      124           
  Lines          6470     6471    +1     
=========================================
+ Hits           5937     5938    +1     
  Misses          533      533           
Files Changed Coverage Δ
src/Command/App/NewFromDrupal7Command.php 77.39% <100.00%> (+0.15%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@wimleers
Copy link
Member Author

wimleers commented Sep 1, 2023

If this gets merged, #1587 can be closed.

@danepowell danepowell removed the bug Something isn't working label Sep 1, 2023
@danepowell danepowell merged commit b2c6811 into main Sep 1, 2023
28 checks passed
@danepowell danepowell deleted the ama-recommendations-default branch February 23, 2024 23:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants