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

feat: add option to change base url of github #124

Merged
merged 1 commit into from
Jan 26, 2022

Conversation

BlackDark
Copy link
Contributor

@BlackDark BlackDark commented Sep 10, 2021

Added new parameter for specifying a different URL for github.
This is useful if a github enterprise instance is hosted and you want to use the code there.

No breaking change.

@gagoar
Copy link
Owner

gagoar commented Jan 8, 2022

Hey, I just saw that, would you like to finish this? I see there are some problems with the workflows, I can fix that!

@gagoar
Copy link
Owner

gagoar commented Jan 8, 2022

Hey @BlackDark I've fixed some of the workflows, to make them run correctly when there's a fork, could you rebase with main? thanks!

Also please add a description of the use case you are trying to get this feature for? thanks,

@BlackDark
Copy link
Contributor Author

Hey @BlackDark I've fixed some of the workflows, to make them run correctly when there's a fork, could you rebase with main? thanks!

Also please add a description of the use case you are trying to get this feature for? thanks,

Hi @gagoar, thanks for the response. Rebased the branch onto the main.
If I need to do sth else I am happy to help!

@codecov
Copy link

codecov bot commented Jan 9, 2022

Codecov Report

Merging #124 (7fda5d9) into main (42e817c) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #124   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            6         6           
  Lines           61        61           
  Branches         8         8           
=========================================
  Hits            61        61           
Flag Coverage Δ
unittests 100.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/commands/getToken.ts 100.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 42e817c...7fda5d9. Read the comment docs.

@gagoar
Copy link
Owner

gagoar commented Jan 9, 2022

Hey @BlackDark I've fixed some of the workflows, to make them run correctly when there's a fork, could you rebase with main? thanks!
Also please add a description of the use case you are trying to get this feature for? thanks,

Hi @gagoar, thanks for the response. Rebased the branch onto the main. If I need to do sth else I am happy to help!

yea, it seems like sadly the DeepSource is failing because the error on the try-catch doesn't have a type. are you familiar with TS to include it in your review? I can wip up a PR fixing that first as well.

Also please add a description to the PR when you get a chance. Is very useful to know the use-case you are optimizing for.

Thanks again.

@gagoar
Copy link
Owner

gagoar commented Jan 10, 2022

The error:
image

Can be solved with:

setFailed(`something went very wrong ${JSON.stringify((e as Error).message)}`);

gagoar added a commit that referenced this pull request Jan 11, 2022
on #124 we seem to have an issue unrelated to the changes that are being made.

I decided to type it so the error stops happening. 

* some upgrades with @typescript-eslint/* that are confusing
@gagoar
Copy link
Owner

gagoar commented Jan 11, 2022

Hey @BlackDark I've fixed the issue you see here coming from DeepSource if you please can rebase your branch and add a description to the PR* we will be good to go!

I can help with the action that uses this package as well so we can release things in tandem.

@BlackDark
Copy link
Contributor Author

Hey @BlackDark I've fixed the issue you see here coming from DeepSource if you please can rebase your branch and add a description to the PR* we will be good to go!

I can help with the action that uses this package as well so we can release things in tandem.

So sorry for the really late action. Didn't have much free time to spare. Rebased and updated the description.
Thanks for providing the fix for the DeepSource issue!

@gagoar gagoar merged commit f5330ac into gagoar:main Jan 26, 2022
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