Fix port detection when Dockerfile declares env vars#23
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #23 +/- ##
==========================================
+ Coverage 67.23% 67.89% +0.66%
==========================================
Files 11 11
Lines 1419 1523 +104
==========================================
+ Hits 954 1034 +80
- Misses 405 423 +18
- Partials 60 66 +6
☔ View full report in Codecov by Sentry. |
Signed-off-by: Philippe Martin <[email protected]>
Signed-off-by: thepetk <[email protected]>
Signed-off-by: thepetk <[email protected]>
Signed-off-by: thepetk <[email protected]>
Signed-off-by: thepetk <[email protected]>
Signed-off-by: thepetk <[email protected]>
Signed-off-by: thepetk <[email protected]>
Signed-off-by: thepetk <[email protected]>
Signed-off-by: thepetk <[email protected]>
Signed-off-by: thepetk <[email protected]>
Signed-off-by: thepetk <[email protected]>
Signed-off-by: thepetk <[email protected]>
Signed-off-by: thepetk <[email protected]>
Signed-off-by: thepetk <[email protected]>
Signed-off-by: thepetk <[email protected]>
Signed-off-by: thepetk <[email protected]>
Signed-off-by: thepetk <[email protected]>
Signed-off-by: thepetk <[email protected]>
Signed-off-by: thepetk <[email protected]>
Signed-off-by: thepetk <[email protected]>
Signed-off-by: thepetk <[email protected]>
Signed-off-by: thepetk <[email protected]>
Signed-off-by: thepetk <[email protected]>
Signed-off-by: thepetk <[email protected]>
Signed-off-by: thepetk <[email protected]>
Co-authored-by: Michael Valdron <[email protected]> Signed-off-by: Armel Soro <[email protected]>
Co-authored-by: Philippe Martin <[email protected]> Signed-off-by: Armel Soro <[email protected]>
… they both work on Dockerfiles Co-authored-by: Philippe Martin <[email protected]> Co-authored-by: Michael Valdron <[email protected]> Signed-off-by: Armel Soro <[email protected]>
|
To move forward on this, I've just pushed the suggested changes. @feloy @michael-valdron Please take a look at the last 3 commits when you get a chance: cc @thepetk |
michael-valdron
left a comment
There was a problem hiding this comment.
Just a few comments to add doc summaries of functions/structures which are external. Other than that everything else looks good to me.
…model.EnvVar' Co-authored-by: Michael Valdron <[email protected]> Signed-off-by: Armel Soro <[email protected]>
Signed-off-by: thepetk <[email protected]>
There was a problem hiding this comment.
@michael-valdron I've updated your review status as I pushed a minor change
I've only updated the name of utils.GetPortsFromDockerfile to utils.ReadPortsFromDockerfile as it was a duplicate name of enricher.GetPortsFromDockerfile, just to avoid potential confusion in the future. Otherwise it looks good to me to be merged.
/lgtm
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: feloy, michael-valdron, thepetk The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
What does this PR do?
This PR introduces a new feature for (java)
micronaut,spring,quarkus, (js)react,express,vueand (PHP)laraveldetectors regarding port values set as env vars inside adockerfile.More detailed, after the check for locally set env vars (into the operating system), if there is no port value detected, we also check if those env vars are set inside a
dockerfile. Thisdockerfileshould be located inside the root dir or one level down.In order to create this functionality we have created
utils.GetValidPortsFromEnvDockerfile(path, []string{envVarNames})which will try to get valid port numbers defined inside a dockerfile. Also theEnvVarstruct is created:Additionally, for
express.jsdetector we have updated the logic, so in case we have a port definition with OR operator we will return both values (env var and default). So for:The necessary updates to the documentation are also included to this PR and along with this new feature we have created multiple test cases for existing and new code.
Which issue(s) does this PR fix
Fixes devfile/api#1223
PR acceptance criteria
Testing and documentation do not need to be complete in order for this PR to be approved. We just need to ensure tracking issues are opened.
Unit/Functional tests
Documentation
How to test changes / Special notes to the reviewer