Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use v1.3.0 in README examples #1

Merged
merged 1 commit into from
Jul 15, 2022
Merged

Conversation

beyarkay
Copy link

@beyarkay beyarkay commented Jul 15, 2022

Thanks for the action!

I got stuck for a while because I copy-pasted the v1.2.0 tag from the readme examples. This PR updates those examples to be 1.2

@IsaacShelton
Copy link
Owner

Oh yeah you're right nice catch! Thank you and merging

@IsaacShelton IsaacShelton merged commit 79a8a84 into IsaacShelton:master Jul 15, 2022
@beyarkay
Copy link
Author

Thanks! I'm really finding the project useful. I don't suppose you know what could be causing an issue like Te [HttpError]: Validation Failed: {"resource":"ReleaseAsset","code":"already_exists","field":"name"} when I'm trying to replace assets on an existing release? (Full logs here).

I've got about 380 assets on one release that I'm trying to update and while I sometimes run into API rate limits, I'm also getting this {"resource":"ReleaseAsset","code":"already_exists","field":"name"} error and I have no idea where its coming from, since I'm pretty sure every asset gets removed before new assets are added.

Link to my personal fork (so I can add debug lines until I find the issue): https://github.com/beyarkay/update-existing-release

Link to the relevant GH action where I use my fork: https://github.com/beyarkay/eskom-calendar/blob/main/.github/workflows/manually_build_calendars.yaml#L32-L40

Link to the GH actions which I've attempted to run, but they're mostly failing (even though they return status code 0): https://github.com/beyarkay/eskom-calendar/actions/workflows/manually_build_calendars.yaml

@IsaacShelton
Copy link
Owner

From the logs it looks like not all the files are being deleted possibly? I don't know about the state that the release was in when it was run, it might have been that the rest didn't exist, but I have a feeling that not all the files are being deleted correctly.

It looks like it deleted up through eastern-cape-peddie.ics and so failed to upload on the one immediately after that (eastern-cape-portalfred.ics) since it thinks it already exists.

In the deleteAssetsIfTheyExist() method of Connection in the main.ts of the action, can you log the list of assets to check if they're correct? My best guess is that somehow those are incomplete. Maybe Github's listReleaseAssets is limited and so doesn't give back everything.

@IsaacShelton
Copy link
Owner

IsaacShelton commented Jul 16, 2022

I looked into the Github REST api a little more and it is limited so that's probably the issue. It looks like it has two parameters, per_page (max 100) and page, which aren't taken into account in the action.

I've made a change that potentially could fix this. If you try using IsaacShelton/update-existing-release@HEAD does that fix the issue?

@beyarkay
Copy link
Author

beyarkay commented Jul 18, 2022

So I've re-run the change with IsaacShelton/update-existing-release@HEAD, and everything looks okay until the release asset id is undefined:


Getting assets for the release...
  Release id: 72143886
  assets:["0","1","2","3","4","5","6","7","8","9","10","11","12","13","14","15","16","17","18","19","20","21","22","23","24","25","26","27","28","29","30","31","32","33","34","35","36","37","38","39","40","41","42","43","44","45","46","47","48","49","50","51","52","53","54","55","56","57","58","59","60","61","62","63","64","65","66","67","68","69","70","71","72","73","74","75","76","77","78","79","80","81","82","83","84","85","86","87","88","89","90","91","92","93","94","95","96","97","98","99","0","1","2","3","4","5","6","7","8","9","10","11","12","13","14","15","16","17","18","19","20","21","22","23","24","25","26","27","28","29","30","31","32","33","34","35","36","37","38","39","40","41","42","43","44","45","46","47","48","49","50","51","52","53","54","55","56","57","58","59","60","61","62","63","64","65","66","67","68","69","70","71","72","73","74","75","76","77","78","79","80","81"]
Deleting old release asset id undefined...
  (node:5905) UnhandledPromiseRejectionWarning: HttpError: Not Found
      at /home/runner/work/_actions/IsaacShelton/update-existing-release/HEAD/dist/main.js:2:50174
      at processTicksAndRejections (internal/process/task_queues.js:97:5)
  (node:5905) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 1)
  (node:5905) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

See the line and workflow here

@beyarkay
Copy link
Author

beyarkay commented Jul 18, 2022

So I think I've found the issue. Or at least, I've got some code that works. My JS isn't too fluent (hence lack of a PR), but this script works:

const { Octokit } = require("@octokit/rest");
const octokit = new Octokit();

const release_id = 72143886;

octokit.paginate(octokit.rest.repos.listReleaseAssets, {
    release_id: release_id,
    owner: 'beyarkay',
    repo: 'eskom-calendar',
}).then(response => {
    assets = [];
    for(let asset of response){
        assets.push(asset);
    }
    console.log(assets[0]);
});

So maybe the change needs to be to remove these lines and replace them with:

let assets = await octokit.paginate(octokit.rest.repos.listReleaseAssets, {
    release_id: release_id,
    owner: 'beyarkay',
    repo: 'eskom-calendar',
});
assets = [];
for(let asset of response){
     assets.push(asset);
}

with the appropriate changes to remove the beyarkay/eskom-calendar specific strings, my JS isn't good enough for me to know that this will work for sure, but I think it will?

@beyarkay
Copy link
Author

What a beautiful green tick: https://github.com/beyarkay/eskom-calendar/runs/7397094554?check_suite_focus=true

It seems like the changes I made worked. Sending a PR through now.

IsaacShelton pushed a commit that referenced this pull request Jul 18, 2022
IsaacShelton added a commit that referenced this pull request Jul 18, 2022
Fix: Asset uploads fail if >30 assets #1
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants