Account created on 26 February 2013, over 12 years ago
#

Merge Requests

More

Recent comments

🇪🇸Spain fjgarlin

I think that "corepack" is modifying the existing "yarn" installation. In fact, if you see "yarn"'s installation instructions here https://yarnpkg.com/getting-started/install, it only requests "corepack"!

As you say, we might not know enough, and to be honest, we don't have to. We can just report what's happening. So I suggest adding _both_ versions of yarn, before and after, and saying "Yarn version after running 'corepack enable'", or something like that.

Setting to NW just based on this last suggestion.

🇪🇸Spain fjgarlin

From the composer.lock file:

        {
            "name": "drupal/webform",
            "version": "6.2.9",
            ...
🇪🇸Spain fjgarlin

I know it's in two places now. We even considered removing it from the composer stage altogether, but the time that we would save on that job might be multiplied in network requests in the other jobs requesting "node_modules".

Re versions, that's so strange! I wonder if it's due to the corepack enable or the @gitlab-formatters/stylelint-formatter-gitlab? Could we debug that?

We could add the version check before the if [ ! -d "$CI_PROJECT_DIR/$_WEB_ROOT/core/node_modules" ]; then (this is what we have now) and after that "if" is closed. That'd mean that it's due to corepack enable. If the versions don't change, then we know it's the yarn add ... line.

If it's the corepack enable, then we'd probably want the "echo" lines after the "if", and not before.

🇪🇸Spain fjgarlin

I left comments and suggestions in the MR. I still prefer the simple check of whether the folder is there or not, regardless of variables or job names. We can still call the snippet in the "composer" job to get the versions output.

🇪🇸Spain fjgarlin

Add new screenshot.

🇪🇸Spain fjgarlin

Add new screenshot.

🇪🇸Spain fjgarlin

If setting the variables on the existing branch of the canonical project works I’d probably opt for that.

Relying on a fork might have more overhead down the line. Now it’s fresh but in a year’s time it might not be so fresh and be more confusing that having an extra (and documented) branch in the canonical project.

In any case, I’ll go with your call as you’re more familiar with the actual day to day and testing of the GTD pipelines.

🇪🇸Spain fjgarlin

Is that “bluehangar” project and exact clone of this template or is there any deviations from this?

Trying to see if adjusting the variable you mention is all it is needed.

🇪🇸Spain fjgarlin

Changes made in https://git.drupalcode.org/project/contribution_records/-/commit/aba2b96...

When a maintainer makes any changes to the attribution checkboxes, a message reminding them to save will be shown:

This is commited to the main branch and it will go with the next deployment, probably tomorrow Monday.

🇪🇸Spain fjgarlin

100%. Not planning to commit anything till next week.

🇪🇸Spain fjgarlin

I placed the echoes in a way that they will always show, regardless of that variable setting.

🇪🇸Spain fjgarlin

This is great news! If the branch is going to stay, then I'd defo change the name to test-only-changes, but I guess that's the plan.

Great work!

🇪🇸Spain fjgarlin

If we want the fix via code, please review https://git.drupalcode.org/project/drupalorg/-/merge_requests/400

Otherwise I guess we can tweak the block visibility settings.

🇪🇸Spain fjgarlin

We can see the output with the versions here https://git.drupalcode.org/project/keycdn/-/jobs/6516217

🇪🇸Spain fjgarlin

I'm working on a fix for contrib to add this feature, so please duplicate instead of moving the issue.

🇪🇸Spain fjgarlin

Note that you are linkinga Drupal core job output. The GitLab CI configuration for core is different that the one of contrib.

Having said that, this is a useful feature for contrib, which we can add, but if you want that added to core, you'll either need to move this issue to the Drupal core queue or create a separate one so it can be managed by core.

🇪🇸Spain fjgarlin

The issue seems to be here

$file = $this->entityTypeManager->getStorage('file')->load($row_data[$key]);

Where $row_data[$key] does not contain the file id.

🇪🇸Spain fjgarlin

fjgarlin created an issue.

🇪🇸Spain fjgarlin

MR ready for review/merge.

🇪🇸Spain fjgarlin

fjgarlin changed the visibility of the branch 3546302-typo-in-banner to hidden.

🇪🇸Spain fjgarlin

Thanks for reporting. Quick change.

🇪🇸Spain fjgarlin

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

🇪🇸Spain fjgarlin

The above MR is merged, which is great, but I found some small issues that still need addressing. I don't think they were hard blockers as we can use many variations which look good, so we can create new MRs with the amends.

I think that the text component or any other text elements in the Section components paragraph need to inherit the Text color setting from the wrapping Section container.

See the three attached images to better understand what I mean. Light text is selected and the text cannot be seen in 2 out of the 3 background variations.

🇪🇸Spain fjgarlin

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

🇪🇸Spain fjgarlin

All user organizations are now migrated via the integrity check + drush command.

🇪🇸Spain fjgarlin

I guess that's where the logic to calculate BASELINE kicks in: https://git.drupalcode.org/project/gitlab_templates/-/blob/main/scripts/...

One part is GitLab CI variables, the other (web) is calculated. Maybe we can add some verbosity there to see if that's the right command. Maybe add a git log before the calculation to see what's available.

🇪🇸Spain fjgarlin

I've integrated the above integrity check into our logs. We will monitor this from now on.
RTBC (not closed) while we monitor things.

🇪🇸Spain fjgarlin

That's a very clever use of the new system (adding comment to the SA, which will transfer the user to the contribution record via webhook), and we don't need to bring backa the old fields and code. Good one!

🇪🇸Spain fjgarlin

fjgarlin changed the visibility of the branch 3545749-organisation-credit-counts to hidden.

🇪🇸Spain fjgarlin

This suggests that the summary is generated only from the first page of the pager.

That is correct. What's shown in that summary is the first page aggregated data, and the first page only includes 5 Drupal core credits.

We cannot get a full count per project without requesting all pages via http requests. The data is correct, it's just the way it's being rendered.

Maybe we can move from:

Documentation, 2 issues
Drupal core, 5 issues

To

Documentation issues
Drupal core issues

or

Documentation (fixed issues)
Drupal core (fixed issues)

And those will be links like this https://www.drupal.org/node/2373279/issue-credits/3060 , where you can see all core issues fixed by Third and Grove.

This would be a quick formatting change, where we remove the innacurate number, and then we can focus the big efforts on 📌 Move issue credit listings to new.drupal.org Active .

🇪🇸Spain fjgarlin

We seem to be in the same situation 2 months after the setting was set to 50 (nearly two months).

Re-opening and setting the value to 0. I will regenerate the whole site again.

🇪🇸Spain fjgarlin

Thanks for reporting. The issue here is that the reported parts where it shows are views. So we might need to add a preprocess field hook or similar.

🇪🇸Spain fjgarlin

Thanks for the suggestion and MR, I think it's great. Setting to RTBC for now.

🇪🇸Spain fjgarlin

Oh, I totally missed that comment. Well spotted.

🇪🇸Spain fjgarlin

This was the last automated credit we had in the old system, as all other automations were removed.

If Simplify agreed commit format Active is done, this might not be needed.

It also has the same challenges as 🐛 Authored-by should use email address from commits Active , as commits only have email which may or may not lead to an actual d.o user. Having said that, this is just one user, so the logic would be easier.

🇪🇸Spain fjgarlin

Before even starting on this, a format needs to be agreed on.
Linking related issue Simplify agreed commit format Active

🇪🇸Spain fjgarlin

The Commits API can return multiple commits per call.

And for each of those commits, we'd need to do an API call to search for the email (we can keep a map so we don't do duplicate calls). It's an API call for each individual email.

The "sync" button won't make any difference with the above calls as all of this (the message generation) is JS driven.

🇪🇸Spain fjgarlin

I always struggle with these too 😅
Maybe just use an "if".

pseudocode:

a = '';
if (sa != '')
  a = a + '|extra'
endif
🇪🇸Spain fjgarlin

The subject and body were taken from the D7 version and changed slightly. Ready for review.

🇪🇸Spain fjgarlin

The two current MRs as similar and against the same branch, which one should I review? (if any).

🇪🇸Spain fjgarlin

Sorry, I missed the "Needs review" here (really busy last 10 days...). Thanks for the work here!

🇪🇸Spain fjgarlin

Disregard the comment. Checking contribution records webhooks.

🇪🇸Spain fjgarlin

...were old issues, but that should not make any difference.

There is a difference with new issues vs old issues where you already are part of it. The setting should persist for brand new issues and new attributions.

But if you were already listed on an issue before September 3td, which is when we did the change, the value for that checkbox is the value that was set in the D7 issue.

We only use defaults when we are bringing a user for the first time into an issue.

I will close it with "Cannot reproduce", but if you notice this in new issues and new attributions, please reopen.

🇪🇸Spain fjgarlin

Note on emails 🐛 Authored-by should use email address from commits Active . Having the no-reply address is easy, possible and straightforward, and would not require any additional api calls. It was initially implemented that way until that issue was created.

Using the actual emails used in the commits (that's what that issue is about) would require an extra api call for every email listed in every MR and keeping a map of users to emails, and then replace those that were matched (which probably most of the time will be the initial no-reply suggestions).

🇪🇸Spain fjgarlin

It seems to also accept secondary addresses.

If we look for @catch's 3 email addresses listed in https://git.drupalcode.org/project/drupal/-/graphs/11.x?ref_type=heads
2 of them work, 1 doesn't. The one not matched seems to be automatically generated by gitlab and not set in a profile (I checked the same scenario with my user).

Going back to #8. We'd need to do an API call for every commit listed on every MR for an issue. Then get a map of email addresses per user and select the most recent.

🇪🇸Spain fjgarlin

@vitaliyb98 - I've run it for your user. Can you try again?

🇪🇸Spain fjgarlin

I've added a new drush command to fix the inconsistencies detected by the integrity check that we added.

I'm going to run that command manually first on a few selected users and if it works as expected, I will add some automation so we can fix directly when detected.

🇪🇸Spain fjgarlin

I'm integrating the new integrity script into our cron rotation.

I will follow up with a drush command to fix the mismatched data.

🇪🇸Spain fjgarlin

Moving to the right project where we build that link.

Issues will be moving very soon to GitLab issues (the new Contribution Record was the final blocker to do this).
I would suggest against making a request to the new site to extract information.

🇪🇸Spain fjgarlin

I've updated both pages. Thanks for raising it!

🇪🇸Spain fjgarlin

Updated information.

🇪🇸Spain fjgarlin

Update screenshot.

🇪🇸Spain fjgarlin

@itamair - more and more actions related to code are being moved to GitLab. Merging MRs has been available for years on the GitLab UI and it is now the only place where MRs can be merged.

As the MR widget was heavily linked with the table of credits, it was removed so the merging workflow goes to GitLab. Issues for projects will be migrated soon.

🇪🇸Spain fjgarlin

Moving to the right project as the data is migrated with the wrong paragraph type.

🇪🇸Spain fjgarlin

I can see an issue with the synced data. I'm looking into this.

🇪🇸Spain fjgarlin

fjgarlin changed the visibility of the branch 3327584-automatic-message to hidden.

🇪🇸Spain fjgarlin

All the planned steps for "before" and "deploy" were done. The "after" clean up tasks will take place in the following days.

🇪🇸Spain fjgarlin

Number of comments yes. Number of commits no. We can also see if files were uploaded and reactions in MRs (eg: thumbs up)

🇪🇸Spain fjgarlin

Replied to the above via slack. Needed to be logged in and click on "Save".

🇪🇸Spain fjgarlin

So, what do you get in CHANGES_TO_REVERT without the wrapping parentheses? Did you output that?

Also, https://www.shellcheck.net/wiki/SC2207 suggests that maybe adding some wrapping double quotes to the part inside the parentheses might fix the issue.

🇪🇸Spain fjgarlin

Disregard comment. Fixing indexing issue.

🇪🇸Spain fjgarlin

Added more details to deployment steps.

🇪🇸Spain fjgarlin

Added a quick cleanup MR to the steps.

🇪🇸Spain fjgarlin

Re 2️⃣, got a +1 via slack from @nod_, @catch, @longwave, and I'll take @poker10 above comment as another +1.

Setting back to RTBC based on that and will wait for a final approval on this (aka mark it as "Fixed").

🇪🇸Spain fjgarlin

Note that the old format is still available in the Contribution Record page ("Not ready to switch...", at the bottom of the page).

I wouldn't like this issue to become as big as their predecessors, because here, it's just about simplifying the format due to the limitations explained in the issue description.

From the link given on #22, there is a neutral "Helped-by" which could also work well here. Tho there seems to be some buy-in for "Co-authored-by".

Given that the suggested format made it to RTBC in #6, and that most the conversation after was about changing "By" to "Co-authored-by" (there were also other questions that were replied), do we have consensus on the simplified format 2️⃣ suggested in the issue description?

Production build 0.71.5 2024