Don't let the app crash on 404s

[fabriziosestito]

This PR adds checks and adds error handling on every parametric route (e.g./hosts/:host)
It also adds tests for every handler.

WIP:

I am not sure on how the NewLandscapeHandler should behave, since it just tries to pull all the environments and then passes them to the view context, which does some filtering if the envName query param is passed or it just shows all the environments otherwise.
See:

trento/web/environments.go

Line 62 in 2c9832f

func NewLandscapeHandler(client consul.Client) gin.HandlerFunc {

I could cycle to check if the landscape exists, but I think I am missing some context here.

Thoughts?

[rtorrero]

@fabriziosestito great work again!

@lee-martin This probably belongs to a different discussion but as they are mentioned here: are we keeping the environments? it might make little sense making changes on them if they are going to go anyway.

[arbulu89]

This PR adds checks and adds error handling on every parametric route (e.g./hosts/:host)
It also adds tests for every handler.

WIP:

I am not sure on how the NewLandscapeHandler should behave, since it just tries to pull all the environments and then passes them to the view context, which does some filtering if the envName query param is passed or it just shows all the environments otherwise.
See:

trento/web/environments.go

Line 62 in 2c9832f

func NewLandscapeHandler(client consul.Client) gin.HandlerFunc {

I could cycle to check if the landscape exists, but I think I am missing some context here.

Thoughts?

Hi @fabriziosestito ,

The why about the sending the environment in the NewLandscapeHandler is that we show the environment which this particular landscape belongs to in the view.
As the environment -> landscape -> system is a hierarchical thing, the landscape itself doesn't store which is its environment. That's why we pass it.
About the filtering, I think this is some leftover, as initially the landscape didn't have to belong to any environment by force. I think we could remove the statement where it checks if the environment is empty.

This way, we need to guarantee that the environment is passed in the query, and the provided c.Param("land") exists in the loaded environments

[fabriziosestito]

About the filtering, I think this is some leftover, as initially the landscape didn't have to belong to any environment by force. I think we could remove the statement where it checks if the environment is empty.

I think that the filtering is still needed, because we are using the sapystems_table partial also in the sapsystems list view, and it expects many or one environment (by filtering).

If I can assume that the envName has to be an existing environment in the NewLandscapeHandler I can just 404 if I find this is not true and keep the template as it is in this PR and we can think maybe to restructure it later.

[stefanotorresi]

LGTM.

[rtorrero]

This looks already like it's good to merge. Thanks!

[stefanotorresi]

gj 🏖️