-
Notifications
You must be signed in to change notification settings - Fork 53
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[DEVHAS-418]add dockerfile as an alternate name for Dockerfile #382
[DEVHAS-418]add dockerfile as an alternate name for Dockerfile #382
Conversation
Signed-off-by: Stephanie <[email protected]>
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #382 +/- ##
==========================================
- Coverage 84.61% 83.89% -0.73%
==========================================
Files 27 31 +4
Lines 3530 4235 +705
==========================================
+ Hits 2987 3553 +566
- Misses 402 515 +113
- Partials 141 167 +26
☔ View full report in Codecov by Sentry. |
cdq-analysis/pkg/detect.go
Outdated
@@ -122,21 +122,22 @@ func search(log logr.Logger, a Alizer, localpath string, devfileRegistryURL stri | |||
} | |||
} | |||
} | |||
} else if f.Name() == DockerfileName { | |||
// Check for Dockerfile | |||
} else if f.Name() == DockerfileName || f.Name() == AlternateDockerfileName { |
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.
Small suggestion: Rather than checking for alternate types of Dockerfile names, what if we convert the name to lower case and check against "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.
then any other mix-case names are also allowed? i.e. 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 guess so? I don’t see the harm in allowing it?
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 we still need to check the exact allowed name, due to
application-service/pkg/devfile/devfile.go
Lines 750 to 756 in 902d897
for _, path := range validDockerfileLocations { | |
dockerfilePath := dir + "/" + path | |
dockerfileBytes, err = DownloadFile(dockerfilePath) | |
if err == nil { | |
// if we get a 200, return | |
return dockerfileBytes, path, err | |
} |
When looking for the files using endpoint, we still need to check for supported names.
But in the cdq analysis here, when we have the repo downloaded on disk, I can convert to lower case and check against that.
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.
updated
Signed-off-by: Stephanie <[email protected]>
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: maysunfaisal, yangcao77 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What does this PR do?:
This PR adds
dockerfile
as an alternate name forDockerfile
also added unit test for it, and update existing test repo for the controller tests.
Which issue(s)/story(ies) does this PR fixes:
https://issues.redhat.com/browse/DEVHAS-418
PR acceptance criteria:
Unit/Functional tests
Documentation
Client Impact
How to test changes / Special notes to the reviewer: