-
Notifications
You must be signed in to change notification settings - Fork 17
Conversation
Did not walk through the code but read you doc:
|
Good points... covered everything except the last one. I'll try to dig on the documentation of other languages but this is something that can be also added later on if the rest works fine. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comments.
Signed-off-by: Luca Stocchi <lstocchi@redhat.com>
Signed-off-by: Luca Stocchi <lstocchi@redhat.com>
Signed-off-by: Luca Stocchi <lstocchi@redhat.com>
Signed-off-by: Luca Stocchi <lstocchi@redhat.com>
Signed-off-by: Luca Stocchi <lstocchi@redhat.com>
Signed-off-by: Luca Stocchi <lstocchi@redhat.com>
Signed-off-by: Luca Stocchi <lstocchi@redhat.com>
@jeffmaury Added options to define port detection strategy to use.
or
If nothing is specified it uses the default "docker, compose, source" |
Signed-off-by: Luca Stocchi <lstocchi@redhat.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strategies should be executed in sequence.
Also HTTP vs HTTPs should be checked
go/docs/port_detection.md
Outdated
Alizer follows the default behavior of Quarkus which reads configuration properties from multiple sources (by descending ordinal). | ||
|
||
1) It checks if the environment variable QUARKUS_HTTP_PORT is set | ||
2) It checks for the `QUARKUS_HTTP_PORT` within the `.env` file, if any, located in the root |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quarkus may use QUARKUS_HTTP_SSL_PORT or QUARKUS_HTTP_PORT or both based on QUARKUS_HTTP_INSECURE_REQUEST.
See https://quarkus.io/guides/all-config and use https as a filter.
for _, algorithm := range settings.PortDetectionStrategy { | ||
ports := []int{} | ||
switch algorithm { | ||
case model.DockerFile: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think all strategies should be used. ex for Quarkus and deploy we can have a default port in source and port in dockerfile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the dotnet _enricher and dot net languages do not have any source detection for the moment. This is why only those 2 algos are in the switch.
For the second point i wonder what's the point of performing all detections.
If you are deploying the app then the port in dockerfile should have priority over the source. Probably it would be the same otherwise there is some issue. If you work locally the one in the source code should be used. By defining the strategy order you should take what you need, no?
The only case it can be useful is if the port in the dockerfile is wrong. E.g it tells to open port 9090 and the source code listen to 9091. in that case it's a user fault.
By executing all strategies we could end up opening more ports than those actually required if some file is dirty.
Signed-off-by: Luca Stocchi <lstocchi@redhat.com>
Signed-off-by: Luca Stocchi <lstocchi@redhat.com>
Signed-off-by: Luca Stocchi <lstocchi@redhat.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
it resolves #81
I added a doc to explain and keep note of how Alizer handles port detection for each framework -> https://github.com/redhat-developer/alizer/blob/34e7afae2ba2aaf4925ce2a554dc27de733c39e3/go/docs/port_detection.md
In few words it does 3 steps:
All yours!
130 files are related to projects used for testing. Everything in that test folder
projectPortTesting
can be skipped.