-
Notifications
You must be signed in to change notification settings - Fork 21
rad-ci: Initial commit #35
base: master
Are you sure you want to change the base?
Conversation
0b24d9c to
abd8fbe
Compare
src/bin/rad-ci.rs
Outdated
| "Couldn't retrieve ssh-agent key #{}", | ||
| options.ssh_index | ||
| ))?; | ||
| agent.userauth(&options.ssh_user, identity)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passing a key index is kind of weird, can we not pass a key path?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
theres ssh which supports key path, but instead of using an index, we could do what other programs do, simply iterate over all keys and return error if all fail. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I think avoiding key path in favor of ssh-agent is a better option, now that I used it)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went ahead and removed index, instead now we iterate over all keys. (Just like ssh itself does)
cloudhead
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is cool, left some comments and will try it out when I have time.
src/bin/rad-ci.rs
Outdated
|
|
||
| let mut remote_file = sess | ||
| .scp_send( | ||
| std::path::Path::new("/etc/concourse-docker-compose.yml"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not in the user's home directory? It's unlikely they can write to /etc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually, I think we should use /app/radicle/etc, what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also problematic, it won't work unless they have a docker install with our docker images.
e1c9771 to
92c6304
Compare
|
|
| use rad_ci::{run, Options, HELP}; | ||
|
|
||
| fn main() { | ||
| rad_terminal::args::run_command::<Options, _>(HELP, "ci", run); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The string here is prepended to failure messages like this: {string} failed: {error}.
So let's set it to just "Command" if this tool can do multiple things, then it'll show Command failed: {error}.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure about this? won't be too informative, also as of now, it's only doing install/uninstalls.
ci/src/lib.rs
Outdated
| receive_hook_content.as_bytes(), | ||
| "/app/radicle/hooks/receieve-hook", | ||
| 0o755, | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not going to work, as you don't know how --root was configured on the server. I think it's best to either have a --remote-root flag to specify it, or to let users install this file manually.
92c6304 to
dd46205
Compare
|
added |
dd46205 to
e87c37d
Compare
e87c37d to
1e0fb53
Compare
Signed-off-by: xphoniex <dj.2dixx@gmail.com>
1e0fb53 to
fc0e9f4
Compare
No description provided.