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

[batch] Allow hailgenetics/vep images to be public #13565

Merged
merged 2 commits into from Sep 7, 2023

Conversation

jigold
Copy link
Collaborator

@jigold jigold commented Sep 6, 2023

No description provided.

danking
danking previously requested changes Sep 6, 2023
Copy link
Collaborator

@danking danking left a comment

Choose a reason for hiding this comment

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

Hmm. We need a new service account without GAR access.

The publicly available images are now the same as the HAIL_GENETICS_IMAGES. Let's change this to explicitly copy over all those images as publicly available ones.

@jigold
Copy link
Collaborator Author

jigold commented Sep 6, 2023

Can you be more specific on what you want the fix to be? Is there an alternate short-term fix here? What is wrong with this change?

@jigold
Copy link
Collaborator Author

jigold commented Sep 6, 2023

The images are already in our artifact registry.

danking
danking previously requested changes Sep 6, 2023
@@ -9,5 +9,7 @@ def publicly_available_images(docker_prefix: str) -> List[str]:
'hailtop',
Copy link
Collaborator

Choose a reason for hiding this comment

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

All the things that we copy over from hail genetics should be public, that's why we copy them over, so let's make that explicit in the code:

from hailtop.batch.hail_genetics_images import HAIL_GENETICS_IMAGES

def publicly_available_images(docker_prefix: str) -> List[str]:
    return [docker_prefix + '/' + image_name for image_name in HAIL_GENETICS_IMAGES]

@danking danking removed the prio:high label Sep 6, 2023
@danking
Copy link
Collaborator

danking commented Sep 6, 2023

removing high priority as I need to fix dataproc first.

@danking danking merged commit 8586275 into hail-is:main Sep 7, 2023
8 checks passed
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.

None yet

3 participants