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

Removed hardcoded git user and protocol #600

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

Conversation

d-ulyanov
Copy link

  1. Git user should not be hardcoded
  2. ssh:// prefix that added inside code doesn't needed in most of cases. Moreover - ssh:// prefix are not supported by some git provider such as bitbucket.
  3. Let user configure git connection completely

@DustyBot
Copy link

Waiting for approval to test from a maintainer.

@thieman
Copy link
Contributor

thieman commented Nov 14, 2015

I'm not sure I understand how this would accomplish your goal of letting the user configure the connection to the remote repo. Would they just put the entire connection string with user into the repo in the app config, e.g. [email protected]:repo? Does that work with these changes?

@d-ulyanov
Copy link
Author

@thieman Yes, you understood it right :) User should put entire connection string. Dusty doesn't have to add default git user to connection. It works for me.

@thieman
Copy link
Contributor

thieman commented Nov 23, 2015

Alright, in that case I don't think we can merge this PR for two reasons:

  1. This seems like it would break existing app/lib configs which rely on the current behavior. Perhaps it could be made to default to the git user if none is provided? Or possibly I am wrong and old specs would continue to work with this. However, the bigger issue is...
  2. The common use case I see for this, which is specifying a custom user to a remote Git server, seems like it would violate the idea of being able to share specs with multiple people. If a property of a specific user is coded into the specs, then the specs are not shareable.

We generally get around the problem in 2 by putting user-specific config into the user's own configuration. The assets feature is a good example of this: you can have a generic app spec which relies on a user's specific SSH key (or other asset) and the separation between shared and private data stays intact: the spec is shared, the asset is private to the user.

To properly ship custom users into Dusty, I think the user would need to be able to specify the Git user on a per-repo basis through user-level config.

@d-ulyanov
Copy link
Author

We have 2 distinct problems that this PR solved:

  1. ssh:// protocol removed from connection string. I think that here we have no problems with backward compatibility.
  2. hardcoded "git" user in connection string. According current code your specs are not sharable too because username "git" already hadrcoded and you can't redefine it using ~/.ssh/config ...
    For backward compatibility we can check if connection string contains "@" - than use it "as is". If no - we can add "git@" before host name. Or we can suggest to add user name to ~/.ssh/config

@thieman
Copy link
Contributor

thieman commented Nov 25, 2015

I'm fine conceptually with 1 if we can come up with enough test cases (doesn't actually have to be in the code in this instance since it's a remote-specific integration test) to convince ourselves it isn't going to break compatibility for anyone. Also I'm curious, which remotes get upset if you include the ssh://? Seems like a very odd thing. If we can do all that, let's put it in a separate PR and try to merge it.

Your point on 2 is taken, though if we are going to fix it we should fix it properly via user-specific config. My guess at what that would involve:

  1. Adding a dusty repos user <repo name> <user name> command to let the user specify which user should be used for a given repo
  2. Adding a field to dusty repos list to indicate which git user is set for each repo
  3. Support for this new information inside the user config

Shouldn't be too hard of a feature to build if you wanted to take a shot at it, or maybe someone else could grab it at some point in the future. @jsingle @paetling thoughts on this approach?

@thieman
Copy link
Contributor

thieman commented Nov 25, 2015

I will also add I am personally in favor of doing this since the hardcoded git user was one of those "let's do the easy thing that works for 95% of people now and fix it later" decisions during initial development.

@paetling
Copy link
Contributor

So what you are suggestion here @thieman is a very specific use case. Users might need to specify which user to pull with from a repo, so make a repos command that allows you to specify this. Is there any thought to making this a more broad feature set? Where you could specify user specific variables and use some sort of syntax to do find and replace with dusty?

@thieman
Copy link
Contributor

thieman commented Nov 25, 2015

@paetling Can you flesh that out more, not sure I can tell exactly what you're getting at

@paetling
Copy link
Contributor

So we could have a set of user defined variables using a dusty user set/unset <variable> commands. These variables could then be used in specs.
For example I could do the following:
dusty set github_user alex

Then in my app specs I could have the following:
repo: <dusty_github_user>@git.hub.github.com

Dusty would then do all the find and replacing before specs were launched. It is more flexible then just adding a command to users, but not sure of a good use case. Maybe user specific env settings you want to pass to a container?

@paetling paetling closed this Nov 25, 2015
@paetling paetling reopened this Nov 25, 2015
@DustyBot
Copy link

Waiting for approval to test from a maintainer.

@paetling
Copy link
Contributor

Sorry about the close, I pressed close and comment by accident.

@thieman
Copy link
Contributor

thieman commented Nov 25, 2015

I think such a system would quickly get unwieldy for variables that aren't top-level. For example, this one really needs to be set on a per-repo basis, so your variables would have to look like <dusty_github_user_repo1> and <dusty_github_user_repo2>. There's also the question of defaults which would be pretty important. It's a complex feature and I'd expect most of the use cases for it could be handled better with a more specialized solution.

@thieman
Copy link
Contributor

thieman commented Feb 12, 2016

@paetling Is this partially addressed by #638?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants