Skip to content

Fix alizer hanging while getting potential dockerfile locations#31

Merged
thepetk merged 5 commits intodevfile:mainfrom
thepetk:fix/fix_get_locations
Oct 4, 2023
Merged

Fix alizer hanging while getting potential dockerfile locations#31
thepetk merged 5 commits intodevfile:mainfrom
thepetk:fix/fix_get_locations

Conversation

@thepetk
Copy link
Contributor

@thepetk thepetk commented Oct 3, 2023

What does this PR do?

This PR creates a separate slice of string inside the getLocation function, in order to avoid looping forever in the locations slice.

More detailed, for a big project, alizer was trying to get a all potential locations of dockerfiles with utils.getLocations. This process tries to locate any Dockerfile/Containerfile inside the root level or one level down. So if the getLocations identifies an item that is a directory inside the root dir of the component it adds this item to the list.

The above process was hanging for a big project as the getLocations was looping in a bigger and bigger slice. By adding the filenames slice and looping for potential filenames there, this issue is now fixed.

Which issue(s) does this PR fix

fixes devfile/api#1279

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

@thepetk thepetk force-pushed the fix/fix_get_locations branch from 3281b7f to fc9c8c5 Compare October 3, 2023 16:50
@codecov
Copy link

codecov bot commented Oct 3, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (4a254ca) 70.42% compared to head (cd5033b) 70.49%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #31      +/-   ##
==========================================
+ Coverage   70.42%   70.49%   +0.07%     
==========================================
  Files          11       11              
  Lines        1562     1566       +4     
==========================================
+ Hits         1100     1104       +4     
  Misses        395      395              
  Partials       67       67              
Files Coverage Δ
pkg/utils/detector.go 86.06% <100.00%> (+0.11%) ⬆️

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

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

Copy link
Member

@rm3l rm3l left a comment

Choose a reason for hiding this comment

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

Also, do you think it could be possible to add a test case to prevent future regressions on this?
I was thinking maybe we could enrich the resources/projects/dockerfile-nested test project with a bunch of new directories. And adding a timeout to the test case to make sure it does not hang. WDYT?

@openshift-ci openshift-ci bot removed the lgtm label Oct 4, 2023
Signed-off-by: thepetk <[email protected]>
@thepetk
Copy link
Contributor Author

thepetk commented Oct 4, 2023

Also, do you think it could be possible to add a test case to prevent future regressions on this? I was thinking maybe we could enrich the resources/projects/dockerfile-nested test project with a bunch of new directories. And adding a timeout to the test case to make sure it does not hang. WDYT?

@rm3l I've added 20 test dirs in the test resource and updated the getLocations by using copy to create a duplicate slice.

Copy link
Member

@rm3l rm3l left a comment

Choose a reason for hiding this comment

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

Thanks for the changes!

@openshift-ci openshift-ci bot added the lgtm label Oct 4, 2023
@openshift-ci
Copy link

openshift-ci bot commented Oct 4, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: michael-valdron, rm3l, thepetk

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [michael-valdron,rm3l,thepetk]

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

@rm3l rm3l added the bug Something isn't working label Oct 4, 2023
@thepetk thepetk merged commit 6b0b633 into devfile:main Oct 4, 2023
@thepetk thepetk deleted the fix/fix_get_locations branch November 9, 2023 10:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved bug Something isn't working lgtm

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Alizer component detection never ends for specific projects

3 participants