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
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,7 @@ target/

# Ignore [ce]tags files
tags

# Idea
.idea
*.iml
11 changes: 10 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,16 @@ You may also *optionally* specify a repository as the second argument. **This is

This resolver will give you access to packages published on *any* repository within the organization. If the token provided in the authentication information only has access to public repositories, then packages published on private repositories will report "not found". If the token has access to private repositories as well as public, then all packages will be visible.

You will need to ensure that `githubTokenSource` is set to *your* details (i.e. the authentication information for the individual who ran `sbt`). The `TokenSource` ADT has the following possibilities:
### Authentication

You will need to ensure that `githubTokenSource` is set to *your* details (i.e. the authentication information for the individual who ran `sbt`). If you do
not establish a token source then you will be unable to authenticate, and you will receive the following message when you attempt to publish:

```
[error] Unable to find credentials for [GitHub Package Registry @ maven.pkg.github.com].
```

The `TokenSource` ADT has the following possibilities:

```scala
sealed trait TokenSource extends Product with Serializable {
Expand Down
2 changes: 1 addition & 1 deletion src/main/scala/sbtghpackages/GitHubPackagesKeys.scala
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ trait GitHubPackagesKeys {

val githubTokenSource = settingKey[TokenSource]("Where to get the API token used in publication (defaults to github.token in the git config)")

val githubSuppressPublicationWarning = settingKey[Boolean]("Whether or not to suppress the publication warning (default: false, meaning the warning will be printed)")
val githubSuppressPublicationWarning = settingKey[Boolean]("Whether or not to suppress a publication warning (default: false, meaning the warning will be printed)")

val githubPublishTo = taskKey[Option[Resolver]]("publishTo setting for deploying artifacts to GitHub packages")
}
Expand Down
6 changes: 3 additions & 3 deletions src/main/scala/sbtghpackages/GitHubPackagesPlugin.scala
Original file line number Diff line number Diff line change
Expand Up @@ -43,14 +43,14 @@ object GitHubPackagesPlugin extends AutoPlugin {
val authenticationSettings = Seq(
githubTokenSource := TokenSource.Environment("GITHUB_TOKEN"),

credentials += {
credentials ++= {
val src = githubTokenSource.value
inferredGitHubCredentials("_", src) match { // user is ignored by GitHub, so just use "_"
case Some(creds) =>
creds
Seq(creds)

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.

}
})

Expand Down