There seems to be a third possible accepted filename per https://phpstan.org/config-reference#config-file
“Needs work” so that can be added too.
On first view (from my phone), it looks really good.
I’ll do proper review and testing on Monday. I appreciate you creating the MR.
This was postponed but was brought in a slack thread. I created an MR with my thoughts from #4 but I'm not sure it can be tested unless merged because of the way it is architected.
I mentioned this in this slack thread https://drupal.slack.com/archives/C51GNJG91/p1763133914571249?thread_ts=...
Just thinking out loud:
- When reading “cached”, read them from a “cached” folder.
- Make the last job in core pipeline (https://docs.gitlab.com/ci/yaml/#stage-post) update the “cached” folder with the newly generated artifacts
That way job will be getting the “cached” version even if a new one is being regenerated
eg (totally untested):
.cspell-cache: &cspell-cache
# Fetch the cspell cache from the artifacts of the latest successful job from
# the target branch. Allow the job to proceed and pass if the file doesn't
# exist.
- 'curl --location --output core/.cspellcache "https://git.drupalcode.org/api/v4/projects/{$CORE_GITLAB_PROJECT_ID}/jobs/artifacts/{$CACHE_TARGET}/raw/cached/core/.cspellcache?job=Lint%20cache%20warming" || true'
...
# This job should be real quick and run at the very end of the pipeline.
.post
script:
- cp -R $CI_PROJECT_DIR/core $CI_PROJECT_DIR/cached/core
artifacts:
paths:
- cached/...
Again, untested, but more promising given the limitation of new variables. Seeing your MR with the "if" in the include gave me the idea.
include:
- project: $_GITLAB_TEMPLATES_REPO
ref: $_GTD_TEMPLATE_REF
rules:
- if: $CI_PIPELINE_SOURCE == 'parent_pipeline'
include:
- project: $_GITLAB_TEMPLATES_REPO
ref: main
rules:
- if: $CI_PIPELINE_SOURCE != 'parent_pipeline'
Feel free to try the above.
Note that "task" is not in the list in the other issue, so we'd need a description for it.
So, how about this?
// Mapped
- Bug report => fix
- Task => task
- Feature request => feat
// Not mapped. Use default: fix
- Support request => fix
- Plan => fix
We can work on proper descriptions for the different types in
✨
Usefull help message or clickable fill for type?
Active
.
Also updated IS with the suggestion and the possible approach in the code.
If we can get a map from the 5 categories we have to a conventional commit type that'd be great.
- Bug report => fix
- Task => ??
- Feature request => feat
- Support request => ??
- Plan => ??
Re #6-#7, I wrote on slack.
Untested:
gitlab_templates > .gitlab-ci.yml
.downstream-base:
...
variables:
# So downstream plugins know which .gitlab-ci.yml include to use.
_GITLAB_TEMPLATES_REPO: $CI_PROJECT_PATH
_GITLAB_TEMPLATES_REF: $CI_COMMIT_REF_NAME
_GTD_TEMPLATE_REF: $CI_COMMIT_REF_NAME
...
Then in gitlab_templates_downstream > .gitlab-ci.yml
variables:
...
_GTD_TEMPLATE_REF: 'main'
include:
- project: $_GITLAB_TEMPLATES_REPO
ref: $_GTD_TEMPLATE_REF
That would set up the correct ref for downstream piipelines and main for “normal” project pipelines
Agree that default to "fix" might make more sense. Done it in the last commit.
There is
🐛
autofill the commit type from the issue category
Postponed
which, if done, might help with setting up the right type automatically.
I don't think this requires "Major" as priority. We have a link to the specification (https://www.conventionalcommits.org/en/v1.0.0) just below the message, where the very first information that you actually find is about commit types. The other link mentioned (https://github.com/pvdlg/conventional-changelog-metahub#commit-types) is not an official one.
There was a suggestion for "Needs review" in #14, but then feedback from #15-#17 brought it back to "Needs work". To move forward, we need some additional JS, or be ok for now with #14.
The JS part could be done in a follow-up, so I'm going to put this again as "Needs review".
This looks good and really simple. Thanks for investigating and fixing this.
RTBC pending the removal of the two testing lines.
Deleted the first revision per @isholgueras as it contained sensitive information.
The only logic about the "new" fields that might be a bit different is in the "presave" hook: https://git.drupalcode.org/project/drupalorg/-/blob/7.x-3.x/drupalorg/dr...
This tries to keep in sync the new fields with the old fields, when possible, and it manipulates the node. This is just a guess.
I only got this behavour when actual comments (automated or by someone) were posted when I tried to save mine. The message was confusing because it mentions the new fields and it probably shouldn't.
We need to investigate more on this.
Also, every contribution record page had a link to this issue, in the part about the old format:
I'd say that the first part of the commit message is now compliant with the conventional commits format (linked in the contribution record pages). I cannot foresee the future, but I think this (small) change is an improvement, and the first part fully adheres to the standard.
All downstream pipelines ran: https://git.drupalcode.org/project/gitlab_templates/-/pipelines/657773
The failed jobs are as expected.
For now, I applied the suggestion. Happy to change to the singular form if that's what's agreed.
The versions between one and the other are very different: https://git.drupalcode.org/project/coder/-/compare/8.3.7...8.3.31?from_p...
That would explain the difference between packages, I guess. But not recognizing the standard "Drupal" is a whole different thing. No idea why that happens.
I'm happy applying the suggestion, but I asked a question before I do it, to be on the safe side.
I'm happy with the code as it is now. So "Needs review" for real 🙂
Re #16, I was confused. Maybe the re-run took the current version of the code, instead of the one of the commit being tested? What I mean by that is that the get-file-via-curl.sh takes the code from the tip of the branch. I was doing/undoing things so that would explain it.
Found the reason and fixed it.
Forced error: https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/72... and https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/72...
For both D7 and D10
No errors in here https://git.drupalcode.org/project/gitlab_templates/-/pipelines/657505
Forced the error in here https://git.drupalcode.org/project/gitlab_templates/-/pipelines/657511 but the error wasn't captured or shown
I gave it a go at it. Please review the solution.
https://git.drupalcode.org/project/gitlab_templates/-/pipelines/657483 has a D7 working pipeline and a D10 one.
Yeah, that's a good point. We know the failures, so we'd easily spot new ones.
I'll make sure to run all downstream pipelines before merging anything from now on.
Well, the idea is that no project will lose any issue, at all. They will just be moved/redirected to GitLab issues.
Happy for GTD to cover the PHP5.6, that's why api-d7 is still there to be honest.
No worries. Supporting PHP5.6 is a kind of edge case at this point, and we have the follow-up issue 🐛 Symlink project script needs to be PHP5.6 compatible Needs work for it.
We could fix the typo before the other issue gets merged in if you want, but if you want to focus on pushing that forward, I'm fine with that too.
I found an issue, but it's unrelated to this work. I opened it here: 🐛 Symlink project script needs to be PHP5.6 compatible Needs work .
I will merge this to main shortly.
Maybe we can use PHP_VERSION_ID to run that code or not or to have different versions of it.
We can perhaps write a version of the code that doesn't use $rest_index and uses the length of the $options array to slice the array.
This code was added in ✨ Add --verbose parameter to symlink_project.php, and a variable turn on verbose Active
I re-ran many other downstream pipelines, and they all run as expected (including the failed jobs).
I've gone through all the changes again, and it looks really good. Mostly, it is the variable calculation and then the replacements all over. I think it's well tested and documented. RTBC.
Right now I'm just trying to keep the projects in the same order as they arrive, but that doesn't mean we will process them that way.
We won't know how many we will do until we start migrating some and see the results. I guess at first we will be more cautious with the first batch, and as the code is refined, we will do more and more.
It's a 1-line MR, I'll do it so it's easy to either merge or iterate on.
The changes are now implemented (commit) and deployed
As this is a core issue and I am not a core maintainer, I won't close it nor assign credit (not all reported users participated). I will leave those tasks to core maintainers.
1. Yes, let's change the docs to use the new variable where appropriate.
2. Happy with that. It should be an easy change at this point.
Adding some of our own DA projects:
-
https://www.drupal.org/project/drupalorg_drupalcon_theme →
(6 issues)
-
https://www.drupal.org/project/drupalorg_migrate →
(29 issues)
-
https://www.drupal.org/project/contribution_records →
(38 issues)
-
https://www.drupal.org/project/drupalorg_crosssite →
(117 issues)
-
https://www.drupal.org/project/gitlab_templates →
(430 issues)
"drupalorg" and "insfrastructure" have almost ~4k and ~5k issues, so not considering those for this initial round.
📌 Decouple issues from www.drupal.org issues (parent and related) Needs review was merged and deployed now.
Only pending deployment step is to remove the issue cockpit from the project's page.
Then we can start migrating projects from the opt-in issue (+200 projects).
I can implement the agreed changes for this as it is RTBC.
As the changes would happen in the "contribution_records" module, should I move this issue there or just leave it here as it is a "core" decision? I am going to do the latter (implement the changes but leave the issue here), but if you feel like the issue needs to move just do it.
I will also wait a few days of the issue being RTBC to make sure the status is not changed back.
Thanks all for the back-and-forth and suggestions, and eventual agreement.
Merged. It will go with the next deployment. Thanks all for the back-and-forth.
Not sure about this one because it’s better that all the logic of each file is completely inside the file, as these are curl’ed into the jobs and separating the logic would make the process way more complex.
I think it’s just coincidental that we have such function duplicated as the cspell and composer php scripts don’t have any overlap.
Unless a good and clean solution is proposed this might be a “won’t change”.
Good suggestion. Validation added for issues and change records link fields.
We can completely hide or disable instead, to have less show/hide elements (we already have messages showing on the credits form).
First load:
After a change is made:
This behaviour is per-form.
The following MR contains the above changes. Please review.
When you make a change, then reveal the button and a “remember to save!” message.
We already have this for the credits table form.
I do like the idea of not showing the save buttons on both forms until changes are made. I will work on something for this.
fjgarlin → changed the visibility of the branch 3556541-attempting-to-create to hidden.
I suggest using the composer command to get this value in the first place rather than analyzing content of files.
Something like this composer config --list | grep bin-dir should give us the output that we need, and it won't matter if it was set up on the file or is the default, it will just output the value.
Updated deployment steps as we no longer need the drush command.
Not a blocker for this, but I will clean up the code not needed.
If you see the conversations about, there is plenty of talk about the ddev plugin.
We removed it because it was no longer needed, everything is calculated beforehand or either via environment variables. The error is informing about what the problem is. I commented on the github issue, all it's needed is to remove the parameter.
That'd be ok for most contributors, though maintainers will get different behaviour when visiting contrib records from their projects vs other projects. I still prefer my suggestion because of this, but I know that it's just a preference so I'm not digging my heels in.
Your solution is easy to implement, so if that's the agreed way forward, I can do it.
We can do that, but I don't know if it'll be more confusing for users, as depending on the contribution record, you might find it open or closed.
Also, most maintainers will go there only to check that the attribution is correct rather than changing it, and in that case, collapsed would also make sense.
A mix between #3 and #4 would be to have it collapsed by defaul and add the "edit" link next to the person, which when clicked, will uncollapse the form.
@drumm - follow-up MR: https://git.drupalcode.org/project/drupalorg/-/merge_requests/414/diffs
This was the section that was assigning colors to the labels. We were fetching just the first page, and with this MR we will loop through all of them.
I went with the latter option:
It will be included in the next deployment to the site.
The first draft implementations of this had a ✔ for those cases.
The only caveat is that if we do this, then the "Git message" JS logic will no longer be dynamic. It will be set to the credited people and if logged out, you would not be able to interact with it. This is the main reason why the form was unified when we made it live.
Disabling the checkboxes should be the quickest/easiest, and we can put a "title" attribute prompting users to log in.
We could even hide it completely, and have a "Edit" button next to the contributor row, and when clicked, it would show the form.
This and the above are really doable, tho in both cases it will require an extra click.
A quick option I experimented with some time ago was to have the attribution form collapsed, like this:
Then it becomes obvious which save button belongs to which section. The logic of the two forms is really different so if something like this would work it'd be be real quick.
I'm going to put a "needs review" for this suggestion, and if accepted, I'll make an MR for it.
I don’t think we need a change record for this.
I will post on #gitlab and we can copy that message to the appropriate channels.
This is merged now! It's a great new feature.
I will tag it later today, make it the new default-ref and then announce it in the #gitlab channel. Thanks both!
Thanks for the screenshots. I think we can just wait until it happens. The output looks great.
> There is one @todo about swapping the URL when the new test is in a the next stable release. Were you thinking that could be done in a follow-up so we can merge this now? Or did you want to wait?
We can actually merge, and the "check-versions.php" job will actually warn us when there is a new release, and that's when we can swap that line. We'll need to remember, but that shouldn't be a problem.
I have given it a final pass at the MR, and it looks good to me. RTBC. Your additions since the previous RTBC are great!
That is correct. Removing those fields will likely make those disappear.
> Migrating this history for the new field would be a nontrivial task, nodechanges stores data as quite an unserialized blob.
This would require a lot more processing per comment, which will add to the total migration time. We'd need to build logic to process the nodechanges field into a sensible "markup".
> Does the migration to GitLab issues preserve nodechanges
No, it only uses nodechanges to detect when the issue was closed or when a file was uploaded, nothing else.
The main MR was merged. Any additional changes will be follow-up MRs, much smaller and easier to review.
From my point of view, things look good. They were good and all the work you've done have brought things to a much better place, better output, more consistency, and more flexibility.
Re the open threads, there is no action needed, just suggestions for namings and possible improvements. I'm happy to iterate a bit more or to leave things as they are.
Command to get the approximate number of issues per module:
drush sql-query "
select fdfpmn.field_project_machine_name_value, count(*) as count
from field_data_field_project fdfp
inner join field_data_field_project_machine_name fdfpmn on fdfpmn.entity_id = fdfp.field_project_target_id
where fdfp.bundle='project_issue' AND
fdfpmn.field_project_machine_name_value in (
'conductor',
'cdn',
-- rest of module machine names from issue description
'navigation_extra_tools'
)
group by fdfpmn.field_project_machine_name_value
order by count, fdfpmn.field_project_machine_name_value;
"
As mentioned, all of them can be easily fixed, except one, and we can use a phpstan ignore clause for that one. In fact, it is already done that way.https://git.drupalcode.org/project/config_notify/-/blob/8.x-1.x/src/Noti...
Your idea is great, but I'd only consider it for a possible 2.x version of the module, keeping the 8.x-1.x small and simple. Feel free to open the issue and work on it if you want.
As for this issue, phpstan job is clean https://git.drupalcode.org/project/config_notify/-/jobs/7145218, so I'm closing it as outdated.
Added contribution records endpoints and remove obsolete information.
Yeah, I've seen it a few times. Everything seems to work tho. Not sure if there is setting that we can do in "composer" to not report that, otherwise we can run the suggested command. We do it here https://git.drupalcode.org/project/gitlab_templates/-/blob/main/scripts/... for example.
Once done, we will probably need to update the documentation as well for external instances. See this commit: https://git.drupalcode.org/project/gitlab_templates/-/commit/39bf8a70cb5...