Skip to content

Fix artifacts url parsing#66

Merged
larsoner merged 10 commits intoscientific-python:masterfrom
matthieutrs:master
Aug 7, 2025
Merged

Fix artifacts url parsing#66
larsoner merged 10 commits intoscientific-python:masterfrom
matthieutrs:master

Conversation

@matthieutrs
Copy link
Contributor

@matthieutrs matthieutrs commented May 5, 2025

This PR proposes a fix for #65. I did not check that this fix still works on the legacy urls, although this should!

Details

In my case,

artifacts_url = `https://circleci.com/api/v2/project/gh/${orgId}/${repoId}/${buildId}/artifacts`;

was not working, the proper url being in the format

artifacts_url = `https://circleci.com/api/v2/project/${projectSlug}/${jobNumber}/artifacts`;

Proposed solution
Check whether the target url fits in the legacy or in the new format, and apply appropriate parsing depending on the case:

if (target.includes('/pipelines/circleci/')) {
      // ───── New GitHub‑App URL 
      // .../pipelines/circleci/<org‑id>/<project‑id>/<pipe‑seq>/workflows/<workflow‑id>
      ...
} else {
      // ───── Legacy OAuth URL (…/gh/<org>/<repo>/<build>) ────────────────
      ...
}

@matthieutrs
Copy link
Contributor Author

matthieutrs commented May 5, 2025

Apologies, closing this as I realize my PR is not appropriately formated. I'll open a better one soon.

@matthieutrs matthieutrs closed this May 5, 2025
@matthieutrs matthieutrs mentioned this pull request May 5, 2025
Closed
@matthieutrs
Copy link
Contributor Author

matthieutrs commented May 5, 2025

Reopening as I could not fix the formatting issue from github. Real diff is around lines 7996 - 8033.

@matthieutrs matthieutrs reopened this May 5, 2025
@larsoner
Copy link
Collaborator

larsoner commented May 5, 2025

Ideally you would edit the root index.js rather than the dist/*, see https://github.com/scientific-python/circleci-artifacts-redirector-action?tab=readme-ov-file#contributing (though it is a bit sparse). Does that make sense?

larsoner and others added 5 commits May 20, 2025 11:27
@larsoner
FIX: Move
c656328
@larsoner
Merge remote-tracking branch 'upstream/master' into mm
d914c02 
* upstream/master:
  Update NCC; pin fetch to v2 (scientific-python#71)
  ENH: Add automatic rebuild (scientific-python#72)
  Build(deps): Bump undici from 5.28.5 to 5.29.0
  Making config example and narrative consistent (scientific-python#67)
@autofix-ci
[autofix.ci] apply automated fixes
71328b1
@larsoner
FIX: Q
fbb367d
@autofix-ci
[autofix.ci] apply automated fixes
0680849
larsoner
larsoner reviewed May 20, 2025
View reviewed changes
index.js
{ headers: { 'Circle-Token': apiToken } }
);
const jobs = await jobsRes.json();
if (!jobs.items.length) {
Copy link
Collaborator

@larsoner larsoner May 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@matthieutrs you had if (!jobs.items?.length) { (note the ?) and eslint complained, so I removed the ?

@larsoner larsoner mentioned this pull request May 20, 2025
Merged
larsoner
larsoner approved these changes May 20, 2025
View reviewed changes
Copy link
Collaborator

@larsoner larsoner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay I pushed this branch to my fork as master and verified that it still worked okay:

https://github.com/larsoner/circleci-artifacts-redirector-action-fork/actions/runs/15142326008/job/42569386101

@matthieutrs can you verify that your fix still works? If not, can you push any necessary changes to index.js and let the autofix.ci bot update dist/index.js?

@larsoner larsoner mentioned this pull request May 20, 2025
Closed
@bsipocz bsipocz added the bug Something isn't working label May 20, 2025
@larsoner larsoner mentioned this pull request May 20, 2025
Merged
@nickodell
Copy link
Contributor

nickodell commented May 21, 2025

Something I want to mention about this is that it won't work in the case where a workflow contains multiple jobs, and the artifacts are produced by a job which is not first in the workflow.

Here is an example value of jobs, to explain what I mean.

{
  "next_page_token": null,
  "items": [
    {
      "job_number": 29,
      "stopped_at": "2025-05-20T02:55:05Z",
      "started_at": "2025-05-20T02:31:58Z",
      "name": "build_scipy",
      "project_slug": "circleci/TZE2qTnF4tD2yK4HmPgb3V/TZakiCrqyJ21zbxV5HZURU",
      "type": "build",
      "requires": {},
      "status": "success",
      "id": "24c4aaeb-5226-4b55-a475-ce7b4edddfa3",
      "dependencies": []
    },
    {
      "job_number": 31,
      "stopped_at": "2025-05-20T02:59:40Z",
      "started_at": "2025-05-20T02:55:08Z",
      "name": "refguide_check",
      "project_slug": "circleci/TZE2qTnF4tD2yK4HmPgb3V/TZakiCrqyJ21zbxV5HZURU",
      "type": "build",
      "requires": {
        "24c4aaeb-5226-4b55-a475-ce7b4edddfa3": [
          "success"
        ]
      },
      "status": "success",
      "id": "0f0cfb08-2cb8-4d4c-a5fd-9f541248bfd7",
      "dependencies": [
        "24c4aaeb-5226-4b55-a475-ce7b4edddfa3"
      ]
    },
    {
      "job_number": 30,
      "stopped_at": "2025-05-20T03:20:25Z",
      "started_at": "2025-05-20T02:55:08Z",
      "name": "run_benchmarks",
      "project_slug": "circleci/TZE2qTnF4tD2yK4HmPgb3V/TZakiCrqyJ21zbxV5HZURU",
      "type": "build",
      "requires": {
        "24c4aaeb-5226-4b55-a475-ce7b4edddfa3": [
          "success"
        ]
      },
      "status": "success",
      "id": "ad088988-083d-4142-9440-655371fdb48e",
      "dependencies": [
        "24c4aaeb-5226-4b55-a475-ce7b4edddfa3"
      ]
    },
    {
      "job_number": 32,
      "stopped_at": "2025-05-20T03:12:01Z",
      "started_at": "2025-05-20T02:55:08Z",
      "name": "build_docs",
      "project_slug": "circleci/TZE2qTnF4tD2yK4HmPgb3V/TZakiCrqyJ21zbxV5HZURU",
      "type": "build",
      "requires": {
        "24c4aaeb-5226-4b55-a475-ce7b4edddfa3": [
          "success"
        ]
      },
      "status": "success",
      "id": "241b350e-8cc1-4b48-b8e0-1e889098e35c",
      "dependencies": [
        "24c4aaeb-5226-4b55-a475-ce7b4edddfa3"
      ]
    }
  ]
}

This code attempts to read the artifacts from build_scipy, even though the actual artifacts are in build_docs job.

@larsoner
Copy link
Collaborator

larsoner commented Aug 7, 2025

@nickodell is that behavior you're describing a regression with this PR? If it's something broken on master and this branch maybe we can merge this as-is (as it fixes some issues) and address it in a follow-up PR?

@nickodell
Copy link
Contributor

nickodell commented Aug 7, 2025 •
edited
Loading

@nickodell is that behavior you're describing a regression with this PR? If it's something broken on master and this branch maybe we can merge this as-is (as it fixes some issues) and address it in a follow-up PR?

This PR doesn't break anything for old style organizations.

On master, new style organizations do not work at all, so it isn't a regression.

@larsoner
Copy link
Collaborator

larsoner commented Aug 7, 2025

Thanks @nickodell !

@matthieutrs good to go from your end / have you had a chance to test following my changes?

@matthieutrs
Copy link
Contributor Author

matthieutrs commented Aug 7, 2025

Hey @larsoner , sorry I've not had the time to review properly but LGTM!

@larsoner larsoner merged commit 2849f4c into scientific-python:master Aug 7, 2025
5 checks passed
@larsoner
Copy link
Collaborator

larsoner commented Aug 7, 2025

Okay let's give it a shot, thanks @matthieutrs !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@larsoner larsoner larsoner approved these changes

Assignees

No one assigned

Labels

bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@matthieutrs @larsoner @nickodell @bsipocz