Skip to content
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

Fix/scan issues #84

Merged
merged 4 commits into from
Sep 13, 2023
Merged

Fix/scan issues #84

merged 4 commits into from
Sep 13, 2023

Conversation

andywaltlova
Copy link
Collaborator

@andywaltlova andywaltlova commented Sep 12, 2023

HMS-2552

  • Fix the possible defects that are related to the code itself of the worker
    • explicit permissions set for temporary folder and check added to close calls
  • Fix the defects that are related to the build process
    • development folder excluded from tarball
  • Figure out a way to fix the false positives that comes from the vendor folder
    • Nothing we can do in a code to deal with them, we will either waive them or mark them as fix later (with intention to fix them in upstream)

src/main.go Outdated
@@ -35,6 +35,7 @@ func main() {

config = loadConfigOrDefault(configFilePath)
log.Infoln("Configuration loaded: ", config)
defer os.RemoveAll(*config.TemporaryWorkerDirectory)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not sure about this, but I noticed that we do not remove the temporaryWorkerDirectory (and any remaining content), what do you think about it @r0x0d ?

Copy link
Member

Choose a reason for hiding this comment

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

I think there are a few things for us to answer here before saying yes or no:

  1. Do we want to remove the directory even if it is user specified?
  2. The contents inside that folder are deleted at the end of the execution, right? So I believe that there won't be anything inside that folder (for now)?

I'm in favor of deleting the temporary directory, as it is really what it proposes, a temporary directory.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah I think it was created with a intention to be only temporary, and you are right that it should be empty, so this should just take care of removing the folder itself

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, if there is nothing else in that folder after we finish the script execution, I'm in favor of removing it. Otherwise, I think we should leave the folder there.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

changed to os.Remove that should keep the folder if something is inside

@codecov-commenter
Copy link

codecov-commenter commented Sep 12, 2023

Codecov Report

Patch coverage: 14.28% and project coverage change: -2.22% ⚠️

Comparison is base (dd0e923) 67.34% compared to head (2a52e7f) 65.12%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #84      +/-   ##
==========================================
- Coverage   67.34%   65.12%   -2.22%     
==========================================
  Files           5        5              
  Lines         343      347       +4     
==========================================
- Hits          231      226       -5     
- Misses         95      101       +6     
- Partials       17       20       +3     
Flag Coverage Δ
go-1.16 65.12% <14.28%> (-2.22%) ⬇️
go-1.20 65.12% <14.28%> (-2.22%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
src/main.go 0.00% <0.00%> (ø)
src/runner.go 78.16% <0.00%> (-3.24%) ⬇️
src/util.go 72.72% <20.00%> (-4.60%) ⬇️

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

Copy link
Member

@r0x0d r0x0d left a comment

Choose a reason for hiding this comment

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

Commented on the threads, but the code looks good so far.

os.Remove instead of os.RemoveAll
@andywaltlova andywaltlova marked this pull request as ready for review September 12, 2023 17:38
@andywaltlova andywaltlova merged commit 22f8dc2 into oamg:main Sep 13, 2023
@r0x0d r0x0d mentioned this pull request Oct 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants