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

feat: implement Julia parser #219

Merged
merged 15 commits into from
Nov 22, 2023
Merged

feat: implement Julia parser #219

merged 15 commits into from
Nov 22, 2023

Conversation

Octogonapus
Copy link
Contributor

This PR implements parsing for Julia Manifests.

I'm adding this as part of aquasecurity/trivy#4438

@Octogonapus Octogonapus changed the title Implement Julia parser feat: implement Julia parser May 22, 2023
@Octogonapus
Copy link
Contributor Author

@knqyf263 This is RTM from my end

@Octogonapus
Copy link
Contributor Author

@knqyf263 Do I need to do anything else on this PR?

@Octogonapus
Copy link
Contributor Author

@knqyf263 @simar7 Any chance this will ever get reviewed or merged?

@knqyf263 knqyf263 requested a review from DmitriyLewen August 16, 2023 08:30
@knqyf263
Copy link
Collaborator

@DmitriyLewen Can you please take a look first?

Copy link
Collaborator

@DmitriyLewen DmitriyLewen left a comment

Choose a reason for hiding this comment

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

Hello @Octogonapus
Thanks for your work!

I left some comments.
Take a look, please.

pkg/julia/manifest/parse.go Outdated Show resolved Hide resolved
pkg/julia/manifest/parse.go Outdated Show resolved Hide resolved
pkg/julia/manifest/parse.go Outdated Show resolved Hide resolved
pkg/julia/manifest/parse.go Outdated Show resolved Hide resolved
pkg/julia/manifest/parse_test.go Outdated Show resolved Hide resolved
pkg/julia/manifest/parse_test.go Outdated Show resolved Hide resolved
pkg/julia/manifest/parse_test.go Show resolved Hide resolved
pkg/julia/manifest/parse_test.go Outdated Show resolved Hide resolved
pkg/julia/manifest/parse.go Outdated Show resolved Hide resolved
@Octogonapus
Copy link
Contributor Author

Thank you for looking at this. Review comments have been addressed.

Copy link
Collaborator

@DmitriyLewen DmitriyLewen left a comment

Choose a reason for hiding this comment

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

@Octogonapus left some idea. Can you take a look?

pkg/julia/manifest/parse.go Outdated Show resolved Hide resolved
pkg/julia/manifest/parse.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@DmitriyLewen DmitriyLewen left a comment

Choose a reason for hiding this comment

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

 left 3 small comments

err = metadata.PrimitiveDecode(dep.Dependencies, &possibleDepsMap)
if err == nil {
possibleUuids := maps.Values(possibleDepsMap)
dep.DependsOn = possibleUuids
Copy link
Collaborator

Choose a reason for hiding this comment

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

If i remember correctly - maps.Values returns values in random order. Can you add sort here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

pkg/julia/manifest/parse.go Show resolved Hide resolved
pkg/julia/manifest/parse.go Show resolved Hide resolved
Copy link
Collaborator

@DmitriyLewen DmitriyLewen left a comment

Choose a reason for hiding this comment

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

Thank you for this work!
Now this PR seems good to me.

@knqyf263 can you take a look and merge this PR, if it is okay for you?

@Octogonapus
Copy link
Contributor Author

@DmitriyLewen @knqyf263 bump

@Octogonapus
Copy link
Contributor Author

@knqyf263 @DmitriyLewen bump, what else is needed for merge?

@DmitriyLewen
Copy link
Collaborator

Hello @Octogonapus
I don't have permission to merge this PR.

@knqyf263 has some priority tasks.
Give us a little more time, please.

@knqyf263
Copy link
Collaborator

We've not seen any demand for this feature so far. Once this PR gets merged, maintainers need to keep maintaining. We probably want to suspend this task until the community needs it.

@Octogonapus
Copy link
Contributor Author

@knqyf263 I understand. In the meantime, I asked the Julia community if others wanted this feature. They have added their support to aquasecurity/trivy#4437

@knqyf263
Copy link
Collaborator

@knqyf263 I understand. In the meantime, I asked the Julia community if others wanted this feature. They have added their support to aquasecurity/trivy#4437

OK, so let's merge your PR! But as we're not so familiar with Julia, we probably need support from the community.

@knqyf263 knqyf263 merged commit fc7f2b4 into aquasecurity:main Nov 22, 2023
@DmitriyLewen
Copy link
Collaborator

Hello @Octogonapus
Can you reopen and update you PR for Trivy now?

@Octogonapus
Copy link
Contributor Author

Thank you!

we probably need support from the community

Yes I can help maintain this.

Can you reopen and update you PR for Trivy now?

Yes I will.

@Octogonapus Octogonapus deleted the julia branch November 22, 2023 22:08
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