Skip to content

Fix port detection when Dockerfile declares env vars#23

Merged
feloy merged 39 commits intodevfile:mainfrom
feloy:pp-1223
Sep 18, 2023
Merged

Fix port detection when Dockerfile declares env vars#23
feloy merged 39 commits intodevfile:mainfrom
feloy:pp-1223

Conversation

@feloy
Copy link
Contributor

@feloy feloy commented Aug 24, 2023

What does this PR do?

This PR introduces a new feature for (java) micronaut, spring, quarkus, (js) react, express, vue and (PHP) laravel detectors regarding port values set as env vars inside a dockerfile.

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. This dockerfile should 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 the EnvVar struct is created:

type EnvVar struct {
	Name  string
	Value string
}

Additionally, for express.js detector 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:

// env var PORT = 3000
var PORT = process.env.PORT || 8080
// Alizer should return []int{3000, 8080}

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

@feloy feloy marked this pull request as draft August 24, 2023 14:03
@thepetk
Copy link
Contributor

thepetk commented Aug 24, 2023

@feloy @rm3l I've finalized the GetEnvVarsFromDockerFile func in order to detect env vars from a dockerfile. I've also fixed the non existing dir path test case

@codecov
Copy link

codecov bot commented Aug 24, 2023

Codecov Report

Patch coverage: 76.92% and project coverage change: +0.66% 🎉

Comparison is base (04361c9) 67.23% compared to head (503680c) 67.89%.

❗ Current head 503680c differs from pull request most recent head 59e606c. Consider uploading reports for the commit 59e606c to get more accurate results

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     
Files Changed Coverage Δ
pkg/utils/detector.go 78.76% <76.92%> (-0.55%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

feloy and others added 25 commits August 30, 2023 16:09
Signed-off-by: thepetk <[email protected]>
Signed-off-by: thepetk <[email protected]>
Signed-off-by: thepetk <[email protected]>
Signed-off-by: thepetk <[email protected]>
@feloy feloy marked this pull request as ready for review August 30, 2023 15:10
rm3l and others added 3 commits September 7, 2023 14:07
Co-authored-by: Michael Valdron <[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]>
@rm3l
Copy link
Member

rm3l commented Sep 7, 2023

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

@rm3l rm3l requested a review from michael-valdron September 7, 2023 12:25
Copy link
Member

@michael-valdron michael-valdron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]>
@rm3l rm3l requested a review from michael-valdron September 7, 2023 14:56
Copy link
Member

@michael-valdron michael-valdron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Sep 7, 2023
@openshift-ci openshift-ci bot removed the lgtm label Sep 11, 2023
Copy link
Contributor

@thepetk thepetk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

cc @feloy @rm3l

Copy link
Member

@michael-valdron michael-valdron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci
Copy link

openshift-ci bot commented Sep 11, 2023

[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.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@feloy feloy merged commit 89027d1 into devfile:main Sep 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Incorrect port detection

4 participants