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

various vulnerability fixes #8950

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

vilay-nference
Copy link

No description provided.

Dockerfile.nfer Outdated
@@ -0,0 +1,40 @@
# Use the specified base image
FROM hz-registry.nferx.com/ubuntu as builder
Copy link
Collaborator

Choose a reason for hiding this comment

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

@vilay-nference Where is the dockerfile for this image? It is not immediately accessible, so we cannot verify its contents.

In general, we use base images provided directly from first-party sources (e.g. https://hub.docker.com/_/ubuntu).

Please change this to use a standard base image and then we can re-evaluate.

Copy link
Author

Choose a reason for hiding this comment

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

This is used for internal purpose. I'll delete this file .. not required for gatk

@@ -232,6 +239,12 @@ configurations {
}

dependencies {
implementation('net.minidev:json-smart:2.4.9') {
Copy link
Member

Choose a reason for hiding this comment

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

Could you explain what these new dependencies are?

Copy link
Author

Choose a reason for hiding this comment

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

This is used as transitive dependencies in one of the package. Overriding the newer version for the compatibles.

Dockerfile.nfer Outdated
@@ -0,0 +1,40 @@
# Use the specified base image
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove this third-party Dockerfile

Copy link
Author

Choose a reason for hiding this comment

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

Yes. will remove this and update you

Copy link
Author

Choose a reason for hiding this comment

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

This is deleted

exclude group: 'net.minidev', module: 'json-smart'
}
// Example dependencies
implementation 'biz.aQute.bnd:biz.aQute.bndlib:5.1.2'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't add new GATK dependencies here unless absolutely necessary

Copy link
Author

Choose a reason for hiding this comment

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

Again dnsjava is used from hadoop client.

Copy link
Author

Choose a reason for hiding this comment

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

@droazen . Let us know if any more modifications required

@lbergelson
Copy link
Member

@vilay-nference Thank you for doing this work. It's nitpicky annoying stuff to figure out.

I have one additional request. Instead of addding additional direct implementation dependencies, could we specify the transtive version requirements in a gradle constraints block?

That will:

  1. make it clear that we don't rely on these directly
  2. prevent us from keeping them around if we do something like remove hadoop in the future
  3. lets us rewrite those force blocks to instead define minimum versions so if the libraries move forward in the future we're not accidentally holding on a to an old version

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.

5 participants