Skip to content
This repository has been archived by the owner on Feb 28, 2021. It is now read-only.

No more questions #119

Merged
merged 9 commits into from
Jun 30, 2017
Merged

No more questions #119

merged 9 commits into from
Jun 30, 2017

Conversation

jastice
Copy link
Contributor

@jastice jastice commented Jun 27, 2017

Removes the prompt from ensuredCredentials and gives information on how to set the credentials instead. This is because the current version uses ensuredCredentials with prompt even when loading the setting, and the whole prompt logic is kind of too messy and there's different ways to set the credentials now.

Also gets rid of most of the printlns by passing loggers around. (addresses #114 #115)

@dwijnand
Copy link
Member

I think prompting is a useful feature: it's more active then just blurting out an error message.

Could you describe the use case that lead you to want to tear this out? There may be a way to support it without this removal.

@jastice
Copy link
Contributor Author

jastice commented Jun 28, 2017

What prompted me to remove the prompt was that it was activated while trying to resolve the credentials in a setting, prohibiting project load. I considered just passing the prompt=false option in this case, but it appears to me it requires too much care in when exactly to use a prompt and when not. Furthermore, when publishing from automated environments, the prompt may lock up the CI build instead of failing fast. Especially at this point the error message with instructions for env vars would be more helpful.

When using sbt-bintray, I personally never found the prompt particularly useful. The process was usually this:

  1. me: Okay, I want to publish
  2. sbt-bintray: nope, credentials, NOW!
  3. me: uuh okay let me look them up. here you go.
  4. sbt-bintray: okay but publish didn't work here's some kind of error
  5. me: oh I must have made a paste error or something
    etc...

Overall, I don't think the added complexity is worth maintaining at this point.

@2m
Copy link
Member

2m commented Jun 29, 2017

I am also for removing this feature.

  • this is the only plugin that I have seen that prompts the user for credentials. Therefore the experience is outside what the user expects.
  • the fact that reload is needed (is it really needed, or is the message that is printed false?) after completing the prompt is evidence that the use case for prompt is a bit involved and against the sbt design.
  • I had troubles writing a scripted test, that tested the case where no credentials were provided. It would prompt for credentials, and immediately complete the prompt with empty string.

@dwijnand
Copy link
Member

I setup my bintray credentials a long time ago, and then versioned them with the rest of my dotfiles, but I'm pretty sure my experience was really pleasant: I ran bintrayChangeCredentials, entered my credentials, ran reload, checked with bintrayWhoami, and I was all done and setup in 5 seconds.

@jastice
Copy link
Contributor Author

jastice commented Jun 29, 2017

bintrayChangeCredentials is an explicit task for changing credentials, this PR doesn't touch it. I only take issue with tasks that you wouldn't expect to pop a prompt on you to suddenly do so.

@dwijnand
Copy link
Member

Because changeCredentials uses ensuredCredentials, wouldn't it spit a warning message now?

@jastice
Copy link
Contributor Author

jastice commented Jun 29, 2017

You're right. I'll fix that.

@jastice
Copy link
Contributor Author

jastice commented Jun 29, 2017

there you go. also included: fix for bintrayUnpublish

@SethTisue
Copy link
Member

+1 to removal

@jastice
Copy link
Contributor Author

jastice commented Jun 30, 2017

If this is ok, I'd appreciate a quick snapshot/gitversion release, as I would like to contribute a continuous deployment process based on it

@dwijnand dwijnand merged commit 0125258 into sbt:master Jun 30, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants