Problem/Motivation

The recipe at /core/recipes/example/recipe.yml isn't a functioning recipe and thus provides a bad example.

It also contains a composer.json file which it doesn't need to.

This was noticed by @jnicola in #3446080: [PP-2] Recipe Browsing UI (comment #10) as it broke recipe discovery.

Steps to reproduce

Try to apply /core/recipes/example

Proposed resolution

Make it a functioning recipe.

Remaining tasks

Fixing it.

CommentFileSizeAuthor
#12 3447994-nr-bot.txt89 bytesneeds-review-queue-bot

Issue fork drupal-3447994

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Issue fork distributions_recipes-3447994

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

thejimbirch created an issue. See original summary.

thejimbirch credited jnicola.

thejimbirch’s picture

thejimbirch commented

thejimbirch opened merge request !138

thejimbirch’s picture

thejimbirch commented
Status: Active » Needs review

Merge request added. I removed the composer.json file, updated the comments, and made a recipe that can be applied that has the defaults that Drupal core installs so it shouldn't change anything if someone applies it.

I tested it by using it with Drupal quick-start

php core/scripts/drupal quick-start core/recipes/example

Josue2591 made their first commit to this issue’s fork.

Josue2591 opened merge request !146

josue2591’s picture

josue2591 commented

Testing process

  1. Install Minimal Profile ddev drush si minimal
  2. Check Initial Setting ddev drush cget text.settings and confirm it is 600 (default)
  3. In the recipe I changed the value of the setting from 600 to 700, to have something different than the default
  4. Run Recipe ddev php core/scripts/drupal recipe core/recipes/example
  5. Check Setting ddev drush cget text.settings and confirm it is now 700

After manually testing it I wrote and included an automated test that does the same thing

ltrain’s picture

ltrain
She/her
Location Toronto
commented
Status: Needs review » Reviewed & tested by the community
thejimbirch’s picture

thejimbirch commented
Project: Recipes Initiative » Drupal core
Component: Code » recipe system
Issue tags: +Recipes initiative

This issue is simple enough. Moving to the core recipe system.

ltrain’s picture

ltrain
She/her
Location Toronto
commented

Great! Just tagging Josué for credit for his review/test.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3447994-nr-bot.txt89 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

thejimbirch changed the visibility of the branch 3447994-example-recipe-isnt to hidden.

thejimbirch’s picture

thejimbirch commented
Status: Needs work » Needs review

Hiding MR 138 as 142 is the correct one.

Moving back to Needs Review to please the bots.

ltrain’s picture

ltrain
She/her
Location Toronto
commented

Can we put it back into RBTC or is there something else needed?

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
commented

Posting a relevant slack thread.

Core contrib question: this issue got moved into Needs Work status by a bot. Is there something we can do to get it back to RBTC?

Laura Johnson It’s a simple issue, I’m unclear on why it was flagged. Although there were two MRs originally so maybe it didn’t like that. The author then hid one of the MRs so maybe we could try putting it back to RBTC…
Michael Lutz Well, one of the MRs has merge conflicts that need to be resolved, that is the one the bot flagged and appears to be the one we don’t want. The other is failing testing, It was created against the distributions_recipies copy of core, is it supposed to be against core now?  It’s hard for me to tell.
Laura Johnson Yeah, it seems like they want us to create MRs against distributions_recipes and then they move stuff that they think is ready to be considered for core (edited)
Michael Lutz I know recipes has an odd workflow with that staging copy of D11, but I don’t know the procedures.
Michael Lutz Either way, the MR is failing tests for some reason, looks to be CI related, like something needs to be rebased somewhere, but I can’t tell where.
Laura Johnson Yes I think there may be another ticket to fix the CI issue
Michael Lutz If it’s supposed to be merged to core, it needs an MR against core.  If it’s supposed to be in d&r, seems like it should be in the d&r queue
Michael Lutz IF the CI failure is in the d&r repo, then that’s why it’s failing.
Michael Lutz If it’s supposed to go straight into core, @Josué needs to switch the MR base to be core, and rebase against core 11.x
Michael Lutz if not, then d&r upstream needs to be fixed before tests will pass and this should merge, though the gates to merge into d&r may not be as strict.
thejimbirch’s picture

thejimbirch commented
Status: Needs review » Needs work

Ah, @mikelutz is correct. THe MR needs to be against core, not the D&R project.

mikelutz changed the visibility of the branch distributions_recipes-3447994-3447994-example-recipe-isnt-with-test to hidden.

mikelutz opened merge request !8432

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
commented
Status: Needs work » Needs review

Alright, lets try that one.

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
commented

Crediting @Josue2591 as he wrote the code. I just opened a new MR against core, cherry picked the commits and (hopefully correctly) fixed some merge conflicts.

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
commented
Priority: Normal » Major

Also switching this to major based on "The major priority is used for issues that are not critical, but that do have significant impact or are important by community consensus." As recipes are a hot item right now I would consider having the example recipe be functional for 10.3.0 to be important, but feel free to reduce if I am wrong.

ltrain’s picture

ltrain
She/her
Location Toronto
commented
Status: Needs review » Reviewed & tested by the community

Thanks Mike! Putting back into RBTC.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

I think the example recipe does not need to be tested by the installer... hopefully we can have a simpler test.

Akhil Babu made their first commit to this issue’s fork.

akhil babu’s picture

akhil babu
he/him
commented
Status: Needs work » Needs review

Added a Kernel test for the example recipe as suggested in #24. Also updated simple_config_update to simpleConfigUpdate in the example recipe.

b_sharpe’s picture

b_sharpe commented
Status: Needs review » Reviewed & tested by the community

Confirmed recipe is functional and only installing optional config when necessary (tested minimal vs standard) since the test is forcing dependencies (search and views). Tests are also passing. RTBC

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed

Backported down to 10.3.x as it's important to have good examples if people are trying to use this feature in 10.3 or 11.0.

Committed and pushed e9574d73b2 to 11.x and aed5e43a78 to 11.0.x and a225c0a38e to 10.4.x and 38ff4e3ace to 10.3.x. Thanks!

  • longwave committed 38ff4e3a on 10.3.x 
    Issue #3447994 by Akhil Babu, Josue2591, thejimbirch, mikelutz, jnicola...

  • longwave committed a225c0a3 on 10.4.x 
    Issue #3447994 by Akhil Babu, Josue2591, thejimbirch, mikelutz, jnicola...

  • longwave committed aed5e43a on 11.0.x 
    Issue #3447994 by Akhil Babu, Josue2591, thejimbirch, mikelutz, jnicola...

  • longwave committed e9574d73 on 11.x 
    Issue #3447994 by Akhil Babu, Josue2591, thejimbirch, mikelutz, jnicola...
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

quietone closed merge request !8432