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

NVSHAS-9584: Update tar.go with using upstream package tarutil #1722

Closed
wants to merge 2 commits into from

Conversation

kyledong-suse
Copy link
Contributor

@kyledong-suse kyledong-suse commented Dec 19, 2024

  1. Remove unmaintained functions in tar.go
  2. Import upstream package tarutil in tar.go
  3. Add upstream package tarutil in vendor

Copy link
Collaborator

@holyspectral holyspectral left a comment

Choose a reason for hiding this comment

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

Generally looks good to me. Some questions about its implementation detail.

share/scan/scan_utils.go Outdated Show resolved Hide resolved
share/scan/scan_utils.go Outdated Show resolved Hide resolved
func (s *ScanUtil) readRunningPackages(id string, pid int, prefix, kernel string, pidHost bool) ([]utils.TarFileInfo, bool) {
var files []utils.TarFileInfo
func (s *ScanUtil) readRunningPackages(pid int, prefix, kernel string, pidHost bool) (tarutil.FilesMap, bool) {
files := make(tarutil.FilesMap)
Copy link
Collaborator

Choose a reason for hiding this comment

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

tarutil.FilesMap is just type FilesMap map[string][]byte

Do we need github.com/coreos/clair/pkg/tarutil for it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that we don't need github.com/coreos/clair/pkg/tarutil. I'll create type FilesMap map[string][]byte for tar processing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After discussed with Jay, I aware that scanner uses most of the tar functions. So it's better to keep this tar.go and update it with using the upstream tarutil.

Copy link
Collaborator

Choose a reason for hiding this comment

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

There seems to be some confusion here. Let's talk through this.

@kyledong-suse kyledong-suse force-pushed the main branch 2 times, most recently from 77fc23e to 45dd755 Compare January 13, 2025 15:10
@kyledong-suse kyledong-suse changed the title NVSHAS-9584: Replace tar.go with upstream package tarutil NVSHAS-9584: Remove tar.go, replace with upstream package archive/tar Jan 13, 2025
@kyledong-suse kyledong-suse force-pushed the main branch 3 times, most recently from 6b026fb to ed21821 Compare January 16, 2025 18:53
@kyledong-suse kyledong-suse changed the title NVSHAS-9584: Remove tar.go, replace with upstream package archive/tar NVSHAS-9584: Update tar.go with using upstream package tarutil Jan 16, 2025
1. Remove unmaintained functions in tar.go
2. Import upstream package tarutil in tar.go
3. Add upstream package tarutil in vendor

Remove tarutil, create type FilesMap map[string][]byte instead

Update tar.go with using upstream package tarutil
Copy link
Collaborator

@holyspectral holyspectral left a comment

Choose a reason for hiding this comment

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

I have concern about tar.go because its copyright from third party is being removed. Please ensure the change is done correctly before merging this PR.

1. Remove unmaintained tar.go
2. Add upstream package github.com/coreos/clair in vendor
3. Create extract.go for NV implemented tar realted functions
@kyledong-suse
Copy link
Contributor Author

Need to separate this PR into 2 part,

  • Remove unmaintained tar.go
  • Add NV implemented tar related functions

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.

2 participants