- Issue created by @adamps
- Status changed to Needs review
about 1 year ago 10:25am 22 October 2023 - last update
about 1 year ago 61 pass - @adamps opened merge request.
- last update
about 1 year ago 61 pass - @adamps opened merge request.
- last update
about 1 year ago 61 pass -
AdamPS β
committed 667b65ec on 4.x
Issue #3395901: Use gitlab-ci
-
AdamPS β
committed 667b65ec on 4.x
- Status changed to Fixed
about 1 year ago 12:25pm 22 October 2023 - Status changed to Needs work
about 1 year ago 5:21pm 22 October 2023 - π¬π§United Kingdom adamps
Strange - it worked here, and on my PC, but it shows as failing at https://git.drupalcode.org/project/simplenews/-/pipelines
I don't seem to have permission to change settings, in particular set the default branch on gitlab.
- π¨πSwitzerland berdir Switzerland
It failed here too. The green results are drupalci, the red x is gitlab.
The difference is almost certainly that gitlab ci defaults to the supported core version, which is 10.1.
Note that removing drupalci.yml doesn't disable those tests, you need to do that on the settings page and instead set up pipeline schedules with variables overides
-
AdamPS β
committed baa4dc4d on 3.x
Issue #3395901: Use gitlab-ci
-
AdamPS β
committed baa4dc4d on 3.x
- π¬π§United Kingdom adamps
It failed here too. The green results are drupalci, the red x is gitlab.
Ah thanks for explaining
Note that removing drupalci.yml doesn't disable those tests, you need to do that on the settings page and instead set up pipeline schedules with variables overides
Thanks I'll do that once it's all working.
The difference is almost certainly that gitlab ci defaults to the supported core version, which is 10.1.
That was my first idea, but apparently not. D10.1 works on my PC, and on Drupal CI see https://www.drupal.org/pift-ci-job/2792108 β .
3.x works on gitlab, see https://git.drupalcode.org/project/simplenews/-/pipelines.
The failures on 4.x are all related to hash authentication. Could there be something strange on gitlab with timestamps or \Drupal::service('private_key')?
- π¨πSwitzerland berdir Switzerland
Hm, I see, I think you have weird encoding issue, looks like the hash can contain special characters?
You can download the artifacts, that contains the browser output HTML files, took me a moment to find a relevant one (you want the last of a test run (specific last ID) for that given test class. For example in Drupal_Tests_simplenews_Functional_SimplenewsSubscribeTest-26-22990325.html, you can see this URL:
GET request to: http://localhost/web/simplenews/confirm/1/1697995771/JuMNJEuYo0UoPdbRXK%...
And that's a 404. That "%80%A6" really doesn't look like something that should go in the URL, you might need to ensure that the hash/key you generate are URL safe? I don't know if the actual difference then is the webserver that's used or if it's just random.
- Status changed to Needs review
about 1 year ago 11:16am 24 October 2023 - π¬π§United Kingdom adamps
Great thanks for the help.
The hash generation code uses
Crypt::hashBase64()
and apparently hasn't changed since 2014. I think the problem might be inextractConfirmationLink()
picking up the wrong link - that function changed a little in 4.x and maybe isn't precise enough now. I don't have any idea why it fails only on gitlab. Let's try like this... - Status changed to Needs work
about 1 year ago 1:31pm 24 October 2023 - π¬π§United Kingdom adamps
Wrong guess. And I accidentally caused a test error, which happily has revealed the full contents of the mail. It's exactly as expected apart from the corrupted URL.
Interestingly the URL ends with %E2%80%A6 which is UTF-8 for "β¦" an ellipsis. Something on the gitlab test run is truncating the URL, but doing it in the wrong encoding. However it seems to work on gitlab for 3.x.
I searched Core/simplenews and the main likely ellipsis is in
Unicode::truncate()
. There are a few places in Core that call that including_filter_url_trim()
.I found simplenews has
MailFormatHelper::absoluteMailUrls()
which looks for "..." - three separate dots, not an ellipsis. So this code is presumably broken because Core uses an ellipsis. But it doesn't seem related to this problem here.I'm running out of ideas it's very mysteriousπ.
- π©π°Denmark ressa Copenhagen
I created #3424725: [Meta] Switching to Gitlab CI β and added this issue, to maybe get some assistance?
- πͺπΈSpain fjgarlin
I'm happy to help here, MR40 has a conflict, so it'd be great to fix that before any further investigation.
Did you know that you can browser or download artifacts generated for the "phpunit" job https://git.drupalcode.org/project/simplenews/-/jobs/216531? See the buttons on the sidebar. It's useful to track which test failed, then browse to that one and see what's on the page. It's very useful for debugging.
One big difference between Drupal CI and GitLab CI is that the former was very forgiving with the links when the site was set up in a subdirectory. GitLab CI is a bit more strict and ensures that links are properly formed (we needed to adapt a few tests in core with this issue). I'm mentioning this because most of the errors are "blablabla was not found anywhere", and that's probably because it ended up in a 404 or something similar.
I'll try to have a look to some of the failing tests and see if I can find something tho.
- πͺπΈSpain fjgarlin
I went to https://git.drupalcode.org/project/simplenews/-/jobs/216531/artifacts/br...
and looked for Drupal_Tests_simplenews_Functional_SimplenewsPersonalizationFormsTest-1-92075051.html, which is the first test run for that file, the one reporting the error. Then I wait for the page to load (https://project.pages.drupalcode.org/-/simplenews/-/jobs/216531/artifact...) and start clicking next, to see all the steps of the test.On the failing step, I see https://project.pages.drupalcode.org/-/simplenews/-/jobs/216531/artifact..., which contains a random string, but doesn't seem to be the same as the expected one. I'd probably add some debug after
$new_value = $this->randomString(20);
to display the contents of the page.I did so here: https://git.drupalcode.org/project/simplenews/-/merge_requests/40/diffs?..., but I can seem to be able to run pipelines in this MR (I reverted the commit). In any case, I was just trying to show what I'd do at this point. The integration seems very close to complete and I feel that once we fix one of those "not found" errors, the others might follow straight away.
If you want more help from my end please say it.
- πΊπ¦Ukraine vlad.dancer Kyiv
vlad.dancer β made their first commit to this issueβs fork.
- Status changed to Needs review
7 months ago 5:09pm 17 May 2024 - πΊπ¦Ukraine vlad.dancer Kyiv
I merged 4.x into MR40 and fixed tests. To address issue with ellipsis reported here #15 I corrected regex to continue looking for a right url. It seems that we have a correct email link, but at the bottom of email:
We have received a request to subscribe xSdv@xzcz.yuk [1] at https://8080-project-drupalpod-8rrv90s5k2j.ws-eu111.gitpod.io/ [2]. To confirm please use the link below. https://8080-project-drupalpod-8rrv90s5k2j.ws-eu111.gitpod.io/simplenew⦠[3] [1] xSdv@xzcz.yuk [2] https://8080-project-drupalpod-8rrv90s5k2j.ws-eu111.gitpod.io/ [3] https://8080-project-drupalpod-8rrv90s5k2j.ws-eu111.gitpod.io/simplenews/confirm/8/1715954779/5AZSMrLlUXg1OAHNKTL4J9uB9qkFeGetnRW8-KqSfdw
- Status changed to Needs work
7 months ago 3:02pm 6 June 2024 - π¬π§United Kingdom adamps
Great nice work - please can you make some quick changes for comments I added in the MR then I will commit
- Status changed to Needs review
7 months ago 9:32pm 7 June 2024 - last update
6 months ago 55 pass, 1 fail -
AdamPS β
committed fa6bbe68 on 4.x
Issue #3395901 by AdamPS: Use Gitlab CI in Simplenews
-
AdamPS β
committed fa6bbe68 on 4.x
- Status changed to Fixed
6 months ago 1:00pm 11 June 2024 - πΊπ¦Ukraine vlad.dancer Kyiv
Hey @AdamPS woud you mind to add git attribution into the commit or at least do this next time?
- π©π°Denmark ressa Copenhagen
It seems like users are no longer automatically checked under the "Credit & committing" section, after they add a commit in an issue ... the same thing happened to me in many issues contained in https://www.drupal.org/project/leaflet_more_maps/releases/2.2.0 β .
I tried to search, but couldn't find an issue, where it was decided to turn off automatic credits after a commit ... Probably to prevent some users from gaming the system? (Which is a real and annoying problem.)
I wonder how we can make sure that maintainers remember to check for credits when changing issue status to "Fixed"?
Note: I am not fishing for credit in this issue, but @vlad.dancer and @fjgarlin should get some credit for their great contributions here with getting Gitlab CI working. And thanks for that!
- πͺπΈSpain fjgarlin
Agree on the fixing of the credits, thanks both for the reminder to @AdamPS.
--It seems like users are no longer automatically checked under the "Credit & committing" section,
It wasn't just one issue. We started turning off the auto-crediting features (mostly due to credit gaming but also to be more in line with the future new system) more than a year ago, in several passes. I can't remember which issues were the ones but I'm sure they'll be in the "drupalorg" queue.
Note that the important part to fill up is the table at the bottom of this issue:
The commit message has not been relevant for credit attribution for several years now. It's all in the above table.
--I wonder how we can make sure that maintainers remember to check for credits when changing issue status to "Fixed"?
We could create an automated message, but that will actually happen when we are in "GitLab issues" as it is already implemented via a webhook when closing the issue. As most of the effort in the coming months will go there I don't know if creating that automatic message in the current issue queue will be a feature request that will be considered. The issue can definitely be created in the "drupalorg" queue.
- π©π°Denmark ressa Copenhagen
Thanks for responding so quickly @fjgarlin. I did consider creating a drupalorg issue, but then didn't do it, since Drupal issues will soon move to Gitlab anyway. So it's probably not worth spending energy on building that feature, and then only use it for a short while.
But really great to hear that crediting is a part of the process of closing an issue on Gitlab.
- π¬π§United Kingdom adamps
Sorry I hadn't realised that it wasn't added automatically.
- π¬π§United Kingdom adamps
I have added credit now. Does that have any effect or is it too late? If too late, is there anything else I can do?
I do appreciate your contribution and I'm sorry if didn't make the correct actions in the system.
- πͺπΈSpain fjgarlin
It's not late at all. As long as the issue is "Fixed", folks can edit their attribution (if they want to attribute work to their organization).
From your side, just checking the checkboxes is enough. Thanks a lot.
Automatically closed - issue fixed for 2 weeks with no activity.
- π©π°Denmark ressa Copenhagen
PS. @fjgarlin, I found the drupalorg issue where automatic crediting was disabled, by leaving all credit checkboxes empty :)