Skip to content
Snippets Groups Projects

#3343522 Move the "do not override" variables into a separate file

Merged #3343522 Move the "do not override" variables into a separate file
1 unresolved thread
1 unresolved thread

Closes #3343522

Merge request reports

Merged results pipeline #164862 passed

Merged results pipeline passed for 71792708

Approval is optional

Merged by Fran Garcia-LinaresFran Garcia-Linares 9 months ago (May 5, 2024 3:10pm UTC)

Pipeline #174085 passed

Pipeline passed for 00fabf43 on main

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
333 333 continue;
334 334 }
335 335
336 // Remaining lines are either values or the name of the variables.
336 // Remaining lines are either the name of the variable, the value or both.
337 337 if (strpos($line, 'value:') === 0) {
338 338 // Remove 'value:' and all quotes.
339 339 $values[$last_name] = trim(str_replace(['value:', '"', "'"], ['', '', ''], $line));
340 340 }
341 341 else {
342 342 $last_name = trim($line, ':');
343 // If the line contains the name and a value then get both.
344 $name_and_value = explode(':', $line);
345 if ($name_and_value[1] != '') {
  • Empty check won't fail if there isn't a value after.

    Suggested change
    Applied
    345 if ($name_and_value[1] != '') {
    345 if (!empty($name_and_value[1])) {
  • Fran Garcia-Linares changed this line in version 5 of the diff

    changed this line in version 5 of the diff

  • I could not make it fail with how the code was before. Even if there is no value, the format of the variables file requires : after the name. So the explode would alway create a second array item. It would only give a PHP Warning: Undefined array key 1 if we had got the format of the file wrong. So the old way would actually alert to an error in the file.

    Edited by Jonathan Smith
  • but it can stay as-is.

  • In that case, if it's needed, and not present, I'd rather throw a hard fail on that, instead of a warning.

  • I think the logic is that if we have:

    variables:
      _NAME:
        value: "..."
        description: "..."

    The first interaction of the loop will capture _NAME under $last_name and then the next iteration captures value: and sets that value under $values[$last_name].

    So the second part of that explode could be totally empty in the example above if we load the variables.yml file. The reason why it might not fail is because we are now loading the hidden-variables.yml file, but I'd rather have the logic support both, which I think it does.

  • Oh, unless explode(':', 'value:') creates array(0 => 'value', 1 => ''). I wasn't sure if it'd create the empty array element, that's why I added the !empty`.

  • So, I'd either leave the code as it is, or do a hard fail if we don't find the :, but that wouldn't even be valid yaml syntax I believe.

  • I think it's good as it is now as you also say above.

  • I wasn't sure if it'd create the empty array element, that's why I added the !empty

    Yes we do get the empty second element when the $line just contains something:

    It worked for both styles of syntax the way I had it, I have tested that. The new bit $name_and_value = explode(':', $line) is done in the else which is when the line does not start with value: so it is only the lines which have the_name: something or just the_name: which are processed here. In either case, we do get the second array item.

    But using !empty is nicer. It won't fail for invalid yml syntax, i.e. if the : was missing, which the previous version did fail on. But we'd soon find out if we missed a : in the file. So let's leave as is.

  • Please register or sign in to reply
  • Fran Garcia-Linares resolved all threads

    resolved all threads

  • added 1 commit

    • bbfad167 - Apply 1 suggestion(s) to 1 file(s)

    Compare with previous version

  • added 2 commits

    • 70d42cf7 - 1 commit from branch project:main
    • fd174f7f - Merge branch gitlab_templates:main into 3343522-hide-variables

    Compare with previous version

  • Fran Garcia-Linares resolved all threads

    resolved all threads

  • Please register or sign in to reply
    Loading