enhancement(queue): don't inject default route

[wass3r]

closes go-vela/community#445

adds another flag to allow not adding the default route. open to alternative naming of the flag.

removes injection of default route for redis queue setup.

IMPORTANT:
since pipelines that don't have a worker declaration automatically get published to the default route: vela, any workers configured with custom routes must also explicitly include vela in their route configuration if the workers are meant to process those workloads.

[codecov]

Codecov Report

Merging #540 (e7638fe) into master (8bb1cf0) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #540      +/-   ##
==========================================
- Coverage   54.79%   54.79%   -0.01%     
==========================================
  Files         178      178              
  Lines       13506    13505       -1     
==========================================
- Hits         7401     7400       -1     
  Misses       5798     5798              
  Partials      307      307              

Impacted Files	Coverage Δ
queue/setup.go	100.00% <ø> (ø)

[jbrockopp]

Do you think it's worth adding a flag for this?

Instead of adding a flag, what if remove the default injection of the vela route?

To be more specific, I'm proposing we remove the following code:

server/queue/setup.go

Lines 40 to 43 in d7a6968

	// check if the default route is provided
	if !strings.Contains(strings.Join(s.Routes, ","), constants.DefaultRoute) {
	s.Routes = append(s.Routes, constants.DefaultRoute)
	}

This would achieve the same end-goal without adding another variable to configure.

[wass3r]

It would turn this into a breaking change since folks will probably have to update worker configs to include vela default route in routes. Otherwise none of the workers will pick up workloads (unless the pipelines target them specifically), right?

[jbrockopp]

It would turn this into a breaking change since folks will probably have to update worker configs to include vela default route in routes. Otherwise none of the workers will pick up workloads (unless the pipelines target them specifically), right?

I don't believe that's the case since we already set vela as the default route:

server/queue/flags.go

Lines 57 to 63 in df1ca62

	&cli.StringSliceFlag{
	EnvVars: []string{"VELA_QUEUE_ROUTES", "QUEUE_ROUTES"},
	FilePath: "/vela/queue/routes",
	Name: "queue.routes",
	Usage: "list of routes (channels/topics) to publish builds",
	Value: cli.NewStringSlice(constants.DefaultRoute),
	},

But if it was a breaking change, I'm not sure that is something that should prevent the change.

[jbrockopp]

Or are you talking about when a worker is already configured with routes that don't include vela?

Meaning those workers would also have to update the configuration to include the vela route?

[wass3r]

Or are you talking about when a worker is already configured with routes that don't include vela?

Meaning those workers would also have to update the configuration to include the vela route?

yes, this. maybe the assumption that most folks set up routes on the worker is too much of an assumption though. i can get behind that.

[jbrockopp]

Or are you talking about when a worker is already configured with routes that don't include vela?
Meaning those workers would also have to update the configuration to include the vela route?

yes, this. maybe the assumption that most folks set up routes on the worker is too much of an assumption though. i can get behind that.

Ok I think I understand and can see the cause for concern 🤔

Unsure what we should do about the assumption of routes configured for workers.

Thinking about this a bit more, I still like the idea of removing the injection of the vela route.

I say this because I could see the cause for confusion from an admin perspective if you configure a worker to listen on specific routes, and unknown to you, the worker is also listening on a route you didn't configure it with (vela).

Thoughts?

FWIW:

On our end, I don't believe we configure the worker routes and we just use the default set by Vela.

[wass3r]

Thinking about this a bit more, I still like the idea of removing the injection of the vela route.

I do as well.

On our end, I don't believe we configure the worker routes and we just use the default set by Vela.

That's fair. At the very least, we would probably want to call the change out in the release notes, since for non-default behavior this could essentially result in non-working.. workers.

[wass3r]

change made 👍🏼

[jbrockopp]

LGTM

[jbrockopp]

I know I approved it already 😞

But could we also remove the routes being declared in the Docker compose file?

https://github.com/go-vela/server/blob/master/docker-compose.yml#L31

https://github.com/go-vela/server/blob/master/docker-compose.yml#L69

[wass3r]

@jbrockopp change made

[kneal]

LGTM 🐬

[jbrockopp]

LGTM