Feature/Develop MVP July 2025 #8
Conversation
Replace FluentAssertions with AwesomeAssertions Move files to libraries when not specific to project
|
I would prefer that the branch is kept reasonably small. I wanted to contribute a basic implementation for user management but now I don't know when/if I should do that (was waiting for the PR to close so that I have a stable code base to work on). Nevertheless this your project and you are free to do whatever you want. |
Understood. This is an unusual "big" PR as there are many changes that I wanted to deliver quickly. It will be merged tomorrow. Sorry for the delay. I will be interested to have your inputs/contributions. Hope you'll appreciate the new changes. I really believe in this project, didn't have the time to work on it before. |
|
| } | ||
|
|
||
| return app; | ||
| app.UseHttpsRedirection(); |
There was a problem hiding this comment.
@devpro This may lead to hard to debug issues as the HTTPS redirection may and will cause required headers to be stripped. Users may be left with the impression that issue is with credentials when the real issue is configuration issue. Also, this may not work with non-standard ports out of the box. Microsoft recommends using HTTPS redirection only with normal websites and advises against it with API sites. The recommended action is not listening at all to HTTP port, or alternatively reacting with HTTP status code 400 (Bad Request).
Also, in some cases users may not have HTTPS available. For example, I will have the certificate system available after I will be able to setup TF and bring up a server which will handle certificates. And, yes I could hack something, but I would like to rather make it work correctly the first time.
There was a problem hiding this comment.
Sorry for the noise, I didn't realize that you had just changed the file to use scoped namespace. But, still, the issues mentioned in my previous comment are valid (if you take out the original thought that redirection is always forced, and thus being configurable).
| /// <param name="tenant"></param> | ||
| /// <param name="name">The name of the Terraform state</param> | ||
| /// <returns>Raw string</returns> | ||
| [HttpGet("{name:regex([[a-zA-Z]]+)}", Name = "GetState")] |
There was a problem hiding this comment.
@devpro Why only a-z (and their uppercase variants)? Wouldn't it be better to use a typed value which will be checked within the type. This will make it testable and most likely more performant as there is no guarantee how many times this regex is created when in the case of typed value it needs to be instantiated only once during the lifetime.
There was a problem hiding this comment.
I didn't find another solution but happy to change.
Problem was to differenciate the calls between {state} and {state}/lock (POST/DELETE).
Features