StrictSlash does not allow to handle error404

[MohamedAshiqrh]

Signed-off-by: Mohamed Ashiq Liyazudeen mliyazud@redhat.com

[MohamedAshiqrh]

please test this @obnoxxx

[MohamedAshiqrh]

review @heketi/dev

[centos-ci]

Can one of the admins verify this patch?

[jarrpa]

What issue does this handle? Are we aiming to return 404 for /hello/, for example?

[MohamedAshiqrh]

@jarrpa not really. This is to make this PR work. removing StrictSlash lets NotFoundHandler to manage the error 404. I am not sure how PR 548 passes tests though.

[jarrpa]

I'll have a look at the tests and see if I can come up with anything. For THIS PR, however, I recommend you amend to reference PR 548 in the commit message.

[obnoxxx]

@centos-ci test this, please

[obnoxxx]

@MohamedAshiqrh indeed, I think we need a better commit message, explaining what the problem is and referencing PR #548 . Still wondering why the unit tests for PR #548 work...

[jarrpa]

The tests pass because they are only testing leaf items, or however they're called. In particular, they're only testing that we return 404 on clusters, nodes, and volumes that don't exist. So we test /clusters/xxx (which doesn't exist), but we don't test, for example, what happens when you go to /clusters/ as opposed to /clusters. Currently /clusters/ just redirects you to /clusters, which I don't think is a bad idea.

With that in mind, I would need a stronger case for this PR before saying we should merge it.

[jarrpa]

Running the latest heketi container image in Kubernetes, I'm seeing the following behavior:

# heketi-cli cluster info fcfd9241ef455dcb1f8dd9c30c3f5ff9
Cluster id: fcfd9241ef455dcb1f8dd9c30c3f5ff9
Nodes:
3aef003008c04312a99017e378969564
49120f7e140d7458a8de07388043c1e5
e66023e1f3ca723c8da0b60ed6ce3655
Volumes:
d9074ae721c50799218d6175f7a18fe9
# heketi-cli cluster info fcfd9241ef455dcb1f8dd9c30c3f5ff9a
Error: Id not found
# heketi-cli cluster info a
Error: Id not found
# heketi-cli cluster info ax
Error: Invalid path or request

This is without @MohamedAshiqrh's patch, and it seems to show the behavior we're looking for already. Is there some other behavior that we're looking to correct?

[obnoxxx]

@jarrpa this actually matches my expectation and understanding of the code. Thanks for testing!
@MohamedAshiqrh I am now still wondering what system / build you tested.

[MohamedAshiqrh]

My Bad, the build was missing the commit. even with out this change and only with that commit this works. Sorry for misleading thoughts.