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

added ghapi support #455

Merged
merged 15 commits into from
Apr 7, 2021

Conversation

pradeepbbl
Copy link
Contributor

Resolves #384

Copy link
Contributor

@kmazurek kmazurek left a comment

Choose a reason for hiding this comment

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

Great start, thanks a lot for your contribution! This works properly when run as part of our integration test suite. :)
Most of the changes look fine, but please take a look at my comments.

Also, I just realised our codestyle CI job is not configured to run on pull requests (again, thanks). You'll need to reformat your code before merging. We use black for formatting and flake8 for linting etc.

Let me know if you have any questions.

goth/runner/download/__init__.py Show resolved Hide resolved
goth/runner/download/__init__.py Outdated Show resolved Hide resolved
goth/runner/download/__init__.py Outdated Show resolved Hide resolved
goth/runner/download/__init__.py Outdated Show resolved Hide resolved
goth/runner/download/__init__.py Outdated Show resolved Hide resolved
@pradeepbbl pradeepbbl force-pushed the enhancement/384-use-ghapi branch from 096dbfe to 86fc687 Compare March 27, 2021 10:30
goth/runner/proxy.py Outdated Show resolved Hide resolved
@pradeepbbl pradeepbbl requested a review from kmazurek March 27, 2021 23:27
@pradeepbbl pradeepbbl changed the title WIP: added ghapi support added ghapi support Mar 27, 2021
goth/runner/download/__init__.py Outdated Show resolved Hide resolved
goth/runner/proxy.py Outdated Show resolved Hide resolved
@kmazurek
Copy link
Contributor

kmazurek commented Apr 6, 2021

btw, let me know if you need my assistance in resolving the conflicts before merging.

@pradeepbbl pradeepbbl force-pushed the enhancement/384-use-ghapi branch from 4ba42a3 to e8be526 Compare April 6, 2021 16:12
@pradeepbbl
Copy link
Contributor Author

nice lint is now running on pull +1

@pradeepbbl pradeepbbl requested a review from kmazurek April 6, 2021 16:22
@pradeepbbl
Copy link
Contributor Author

is something wrong with the test suit was running for the last 1hr?

@kmazurek
Copy link
Contributor

kmazurek commented Apr 6, 2021

Yes, I stopped the job and cleaned our test runners. Could you please merge the repo's latest master into this branch and try again?

@pradeepbbl pradeepbbl force-pushed the enhancement/384-use-ghapi branch from 813af02 to a6eee83 Compare April 6, 2021 20:39
@pradeepbbl
Copy link
Contributor Author

pradeepbbl commented Apr 6, 2021

Not sure looks like something wrong with CI rebase the repo to upstream/master and ran both black and flake8 no issues but failing in runners

$ git rebase upstream/master
Current branch enhancement/384-use-ghapi is up to date.
$ black  .
All done! ✨ 🍰 ✨
77 files left unchanged.
$ flake8 .

@kmazurek
Copy link
Contributor

kmazurek commented Apr 7, 2021

@pradeepbbl I ran black locally on your latest branch and I'm getting the same errors as reported by CI. Can you please verify your black version? You can do so by running black --version. The project is configured to use version 20.8b1, you should have the same one if you installed it through poetry along with goth.

Otherwise the PR looks good to go! :)

@pradeepbbl
Copy link
Contributor Author

@zakaprov thanks for the help

Copy link
Contributor

@kmazurek kmazurek left a comment

Choose a reason for hiding this comment

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

LGTM! 🥳

Thanks a lot for your contribution! I'll try to tag some more "good first issues" in the repo in case you're interested. :)

@kmazurek kmazurek merged commit 6d1a271 into golemfactory:master Apr 7, 2021
@pradeepbbl
Copy link
Contributor Author

you can directly assign to me if you want ;)

@pradeepbbl pradeepbbl deleted the enhancement/384-use-ghapi branch April 7, 2021 15:35
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.

Use ghapi in GithubDownloader
2 participants