Skip to content

Conversation

@Lavaerius
Copy link

Description:

Updates fail condition for getManifestFromRepo function to include instances when github responds with JSON body for API rate limiting, that is incorrectly handled.
Related issue:

#903

Check list:

  • Mark if documentation changes are required.
  • Mark if tests were added or updated to cover the changes.
@Lavaerius Lavaerius requested a review from a team as a code owner September 17, 2024 19:31
}
}
return releases;
if (!releases.hasOwnProperty('documentation_url')){
Copy link
Contributor

Choose a reason for hiding this comment

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

Dist/index.js is not where these changes should be directly made.

This index.js is a form of webpack and gets auto-generated from the source files in the src directory.

"release": "ncc build -o dist/setup src/setup-python.ts && ncc build -o dist/cache-save src/cache-save.ts && git add -f dist/",

Fundamentially you're trying to update the getManifestFromRepo function. That can be found here:

export function getManifestFromRepo(): Promise<tc.IToolRelease[]> {
core.debug(
`Getting manifest from ${MANIFEST_REPO_OWNER}/${MANIFEST_REPO_NAME}@${MANIFEST_REPO_BRANCH}`
);
return tc.getManifestFromRepo(
MANIFEST_REPO_OWNER,
MANIFEST_REPO_NAME,
AUTH,
MANIFEST_REPO_BRANCH
);
}

That uses a function from the our tool-cache NPM package in the actions/toolkit repository. This is the actual place where the change would need to be made: https://github.com/actions/toolkit/blob/6dd369c0e648ed58d0ead326cf2426906ea86401/packages/tool-cache/src/tool-cache.ts#L589-L632

So upstream the change would need to be made to tool-cache, a new version would then need to be published and then pulled into this repository/action. https://www.npmjs.com/package/@actions/tool-cache

I recommend submitting a PR to https://github.com/actions/toolkit

Copy link
Author

Choose a reason for hiding this comment

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

Appreciate the feedback on this issue. I'll investigate your recommendation.

@mahabaleshwars
Copy link
Contributor

Hello @Lavaerius,

Are you planning to make modifications to this PR as per @konradpabjan's suggestion?
Please update your input so that we can move this forward and resolve the issue.

Thank you!

@Lavaerius
Copy link
Author

Yes, I'll look into that.

@Lavaerius
Copy link
Author

I've submitted:
actions/toolkit#1832

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

Labels

None yet

3 participants