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

Create a library that can be imported #5

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Create a library that can be imported #5

wants to merge 4 commits into from

Conversation

Titaniumtown
Copy link

I whipped up this "library version" of main.py which can be imported. So one could just import it like: from artifact_lib import get_latest_artifact_url

@Titaniumtown
Copy link
Author

Titaniumtown commented Aug 24, 2020

I've added a check for seeing if you've hit your rate limit for the github api. Should be a temp fix for errors thrown by #4

@JayFoxRox
Copy link
Owner

This PR adds a lot of redundancy. However, adding redundancy is not acceptable in my opinion (especially if we have issues like #4 or #3 which now affect the duplicated code).
Instead, this should replace the existing main.py (or slim it down to where it only uses this library) and deal with packaging (like a typical Python module / library).

An API might also not be suitable until we figure out #1 so that the API could remain stable.
I see that you added the parameters to the functions, but that doesn't feel like the Python-way to go about it.
It was fine for a dirty hack like the original tool, but it's not acceptable for a long-term solution like a Python module / library.

@Titaniumtown
Copy link
Author

This project is just something I'm using for a little project of mine (https://github.com/Titaniumtown/yatopia-autoupdate) a minecraft server updater, that auto-fetches the latest artifact of a specific branch.

@JayFoxRox
Copy link
Owner

JayFoxRox commented Aug 24, 2020

That's all fine, but I don't think your changes are upstreamable (to this project) for the reasons I described before.
I'm suprised that your script works for you (does it? how do you authenticate?)

With the master branch, I keep getting this for my own repositories (which is the issue described by #4):

{
  "message": "You must have the actions scope to download artifacts.",
  "documentation_url": "https://docs.github.com/rest/reference/actions#download-an-artifact"
}

So the HTTP server would have to be a logged in GitHub User of some form, which is not acceptable to me.
Even if that was done, we'd be running into actions/upload-artifact#51 (which means any generated URLs don't work, unless we act as a proxy for guest users).

I don't see how your modifications would work around this problem, or how your repository can be configured to avoid this?

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