Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add support for targetPort #182

Merged
merged 1 commit into from
Mar 7, 2022
Merged

Add support for targetPort #182

merged 1 commit into from
Mar 7, 2022

Conversation

c4710n
Copy link
Contributor

@c4710n c4710n commented Jan 9, 2022

Allow to specify remote info like this:

{
  deployment.targetHost = "11.11.11.11";
  deployment.targetUser = "deploy";
  deployment.targetPort = 8022;  # add this
}

Then:

  • there's no need to use SSH_CONFIG_FILE in a simple case which just specifies a non-standard port.

I'm not an expect of Go. If you have any idea of the code, just change it. I'm ok with that. ;)

@srhb srhb added the needs-review Want to merge, team needs to test and review label Mar 3, 2022
@srhb srhb added triaged Discussed in-team, actionable and removed needs-review Want to merge, team needs to test and review labels Mar 4, 2022
@c4710n c4710n force-pushed the target-port branch 5 times, most recently from 0719611 to f2dffb0 Compare March 4, 2022 13:52
@c4710n
Copy link
Contributor Author

c4710n commented Mar 4, 2022

I compared host.TargetPort and host.GetTargetPort() with the zero-value of int - 0.

I'm not sure whether it is a good taste of Go code. @ me if more changes are required.

@srhb
Copy link
Contributor

srhb commented Mar 7, 2022

I think it's fine even though we're relying on unmarshal defaulting to 0. If not we can always add some proper optional'ing later on.

Thank you very much! :)

@srhb srhb merged commit 5b85237 into DBCDK:master Mar 7, 2022
@c4710n c4710n deleted the target-port branch March 12, 2022 09:53
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
triaged Discussed in-team, actionable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants