Skip to content

Enable use of hostname from nats links #530

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Enable use of hostname from nats links #530

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c811f54
Enable use of hostname from nats links
andrew-edgar Jun 30, 2020
84d77de
Fix to add the else_if_link for nats hostname
andrew-edgar Jul 10, 2020
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
11 changes: 9 additions & 2 deletions jobs/route_emitter/templates/route_emitter.json.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,12 @@
nats_machines = nil
if_p('diego.route_emitter.nats.machines') do |ips|
nats_machines = ips.compact
end.else_if_link('nats') do
nats_machines = link('nats').instances.map { |instance| instance.address }
end.else do
Copy link
Contributor

@flawedmatrix flawedmatrix Jul 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if getting rid of the if_link('nats') check would break anything

Copy link
Contributor Author

@andrew-edgar andrew-edgar Jul 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is not really an issue. The previous code if there was no nats link and no nats.machines specified would set nothing for the nats addresses. And the route_emitter would fail connecting to nats.

Now this code will, if there is no nats.machines specified and there is no nats link, fail trying to get the nats link on line 8.

Both will have the route_emitter failing. This code is exactly the same as what the route_registrar does and that is why I implemented it as such.

If you want the route_emitter to fail the same way it does now instead of failing this new way we could change line 7 to match the end.else_if_link('nats') do

But I do feel that it is better to fail during the deployment rather than have a successful deploy that when the route_emitter starts will not be able to connect to nats

Copy link

@univ0298 univ0298 Jul 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand Andrew correctly, previously, if there was no nats link and no nats.machines specified, then the code would set nothing for the nats addresses. And then the route_emitter would subsequently fail at runtime because it would be unable to connect to nats as there were no machines/addresses.
This PR will change the behavior so that it will fail at deploy time because it cannot get the required nats link. That is better than failing at runtime

link('nats').if_p("nats.hostname") do |hostname|
nats_machines = [hostname]
end.else do
nats_machines = link('nats').instances.map { |instance| instance.address }
end
end

nats_port = nil
Expand All @@ -32,6 +36,9 @@
if_link('nats-tls') do |nats_link|
if p("diego.route_emitter.nats.tls.enabled")
nats_machines = nats_link.instances.map { |instance| instance.address }
nats_link.if_p("nats.hostname") do |hostname|
nats_machines = [hostname]
end
if_p("diego.route_emitter.nats.tls.hostname") do | prop |
nats_machines = [prop]
end
Expand Down
11 changes: 9 additions & 2 deletions jobs/route_emitter_windows/templates/route_emitter.json.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,12 @@
nats_machines = nil
if_p('diego.route_emitter.nats.machines') do |ips|
nats_machines = ips.compact
end.else_if_link('nats') do
nats_machines = link('nats').instances.map { |instance| instance.address }
end.else do
Copy link
Contributor

@flawedmatrix flawedmatrix Jul 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Copy link
Contributor Author

@andrew-edgar andrew-edgar Jul 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment as above

link('nats').if_p("nats.hostname") do |hostname|
nats_machines = [hostname]
end.else do
nats_machines = link('nats').instances.map { |instance| instance.address }
end
end

nats_port = nil
Expand All @@ -32,6 +36,9 @@
if_link('nats-tls') do |nats_link|
if p("diego.route_emitter.nats.tls.enabled")
nats_machines = nats_link.instances.map { |instance| instance.address }
nats_link.if_p("nats.hostname") do |hostname|
nats_machines = [hostname]
end
if_p("diego.route_emitter.nats.tls.hostname") do | prop |
nats_machines = [prop]
end
Expand Down