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

Credentials should remain optional #34

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

huntc
Copy link

@huntc huntc commented Feb 4, 2021

When publishing with sbt, I believe credentials are only required when the endpoint receiving its artifacts requires them. This PR re-enables this behaviour. As a consequence, the plugin may be used in situations where the user has no credentials given that they are unauthorised to publish artifacts. A valid scenario is that Continuous Delivery is authorised to publish, but no other user.

The plugin also now works in the context of an IDE where the GITHUB_TOKEN environment variable is not found.

Should address #30, #28, #26, #16 - but please re-check.

With sbt publishing, credentials are only required if the endpoint receiving the artifacts requires them. This commit therefore retains this behaviour. As a consequence, the plugin may be used in situations where the user has no credentials given that they are not authorised to publish artifacts. A valid scenario is that Continuous Delivery is authorised to publish, but not an individual developer.
@l15k4
Copy link

l15k4 commented Feb 26, 2021

Thanks for this PR, especially when the githubTokenSource cannot be defined in multi-module project as githubTokenSource in ThisBuild or githubTokenSource in Global simply won't help, one has to declared it in each individual project for some reason

@nightscape
Copy link

@djspiewak could this be merged?
It's rather annoying to work around this issue when one is not currently requiring a GITHUB_TOKEN.

@l15k4
Copy link

l15k4 commented Mar 1, 2021

I agree, pretty please @djspiewak 🙏🏼 With bintray being closed soon, a lot of people might start porting to github packages. But this is really essential change.

@djspiewak
Copy link
Owner

Sorry I've been super-swamped! Catching up on all of this.


case None =>
sys.error(s"unable to locate a valid GitHub token from $src")
Seq.empty
Copy link
Owner

Choose a reason for hiding this comment

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

So the problem I have with this is the fact that resolution from GitHub Packages also requires a token. You can't just download a package unauthenticated, meaning that credentials are necessary at all times regardless of whether or not you're publishing. Honestly, this is a thing that GitHub needs to fix.

Copy link
Author

Choose a reason for hiding this comment

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

Won’t the authentication issue arise at the time of resolution or publication though ie aren’t we doubling up on the checking here?

Copy link
Owner

Choose a reason for hiding this comment

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

It would arise at the time of resolution. Basically this is trying to give users a better error message. They won't be able to use the plugin to either publish or resolve without a token, so just trying to give a message which is semantic rather than some opaque "could not resolve artifact from one of these resolvers" error.

Copy link
Author

Choose a reason for hiding this comment

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

I’m not sure that this is the correct place to improve on resolution messages, but your call. Happy for you to close this PR though.

@hejfelix
Copy link

What happened to this? Without it, bloopInstall seems to always fail unless you add the token to your global env.

@mhammons-ledger
Copy link

Seconding the need for this. My coworkers are offput by the need to have a github token provided to SBT to do anything, even compile. Please note that in the case of resolution of dependencies, if the user only publishes to github then it shouldn't be a problem to defer credentials checks till publishing is attempted.

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.

6 participants