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

Transfer images #1

Merged
merged 27 commits into from
Jul 14, 2022
Merged

Transfer images #1

merged 27 commits into from
Jul 14, 2022

Conversation

iameskild
Copy link
Member

@iameskild iameskild commented May 31, 2022

As we transition to the nebari-dev org, we have now decided to split out the docker images from the old monorepo.

Fixes: nebari-dev/nebari#1174

OUTDATED

A few outstanding items or questions:

  • Can I have my permissions for this repo elevated so I can add the appropriate secrets?
  • I tried to update the image.yaml workflow to only push to GHCR and Quay but it seems that the docker/build-push-action will always try to push to Dockerhub. GHA details in my fork.
    • This means we have to either find a replacement for the docker/build-push-action workflow or registry a Dockerhub account for nebari. Thoughts?

@iameskild
Copy link
Member Author

iameskild commented May 31, 2022

Part of an on-going effort to transfer the repo from qhub to nebari-dev:
nebari-dev/nebari#1292

@iameskild iameskild requested review from trallard and magsol June 2, 2022 22:02
@iameskild
Copy link
Member Author

iameskild commented Jun 2, 2022

@costrouc @trallard @magsol this PR is ready for review. I tested it on my forked repo and the images were successfully built and pushed.

Copy link
Contributor

@magsol magsol left a comment

Choose a reason for hiding this comment

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

LGTM. May still want to wait for an additional review but they build successfully for me.

.github/workflows/image.yaml Outdated Show resolved Hide resolved
.github/workflows/image.yaml Outdated Show resolved Hide resolved
.github/workflows/image.yaml Outdated Show resolved Hide resolved
.github/workflows/image.yaml Outdated Show resolved Hide resolved
.github/workflows/image.yaml Outdated Show resolved Hide resolved
jupyterhub/postBuild Show resolved Hide resolved
scripts/install-conda.sh Outdated Show resolved Hide resolved
scripts/install-conda.sh Outdated Show resolved Hide resolved
scripts/install-conda.sh Outdated Show resolved Hide resolved
scripts/install-conda.sh Show resolved Hide resolved
@trallard trallard added needs: changes 🧱 Review completed - some changes are needed before merging and removed needs: review 👀 This PR is complete and ready for reviewing labels Jun 8, 2022
@trallard
Copy link
Member

trallard commented Jun 8, 2022

@iameskild, thanks for opening this PR. I just went through an in-depth review and left several comments.

Some additional thoughts:

  1. Right now the Dockerfiles are in . but there are directories for each image - this pattern is counter-intuitive I'd suggest moving the Docker images to the respective directories to keep everything aligned
  2. Additionally - and moving forward it be best to give more details on the infrastructure/ config of the repos this can be done either:
    1. Adding a Config files doc (.md) like the repo2docker one
    2. Adopting an INFRASTRUCTURE.md like the sphinx-book-theme one, pymc

Let me know if there is anything that needs clarification or else 👋🏽

Resources

@trallard
Copy link
Member

trallard commented Jun 8, 2022

  • Can I have my permissions for this repo elevated so I can add the appropriate secrets?

I believe this has been done? When adding secrets, can we create an env such as docker-dev then place the secrets there?

  • I tried to update the image.yaml workflow to only push to GHCR and Quay but it seems that the docker/build-push-action will always try to push to Dockerhub. GHA details in my fork.

I went through the logs, and it seems the error is Error: buildx failed with: error: failed to solve: server message: insufficient_scope: authorization failed
so that makes me believe you need to change the scope of the PAT - and this might be related to you trying to login to the nebari ghcr from your personal fork which I assume might be a no no

.github/workflows/image.yaml Outdated Show resolved Hide resolved
.github/workflows/image.yaml Outdated Show resolved Hide resolved
.github/workflows/image.yaml Outdated Show resolved Hide resolved
iameskild and others added 2 commits June 16, 2022 16:34
Co-authored-by: Tania Allard <taniar.allard@gmail.com>
Co-authored-by: Tania Allard <taniar.allard@gmail.com>
iameskild and others added 13 commits June 16, 2022 16:35
Co-authored-by: Tania Allard <taniar.allard@gmail.com>
Co-authored-by: Tania Allard <taniar.allard@gmail.com>
Co-authored-by: Tania Allard <taniar.allard@gmail.com>
Co-authored-by: Tania Allard <taniar.allard@gmail.com>
Co-authored-by: Tania Allard <taniar.allard@gmail.com>
Co-authored-by: Tania Allard <taniar.allard@gmail.com>
Co-authored-by: Tania Allard <taniar.allard@gmail.com>
Co-authored-by: Tania Allard <taniar.allard@gmail.com>
Co-authored-by: Tania Allard <taniar.allard@gmail.com>
Co-authored-by: Tania Allard <taniar.allard@gmail.com>
Co-authored-by: Tania Allard <taniar.allard@gmail.com>
Co-authored-by: Tania Allard <taniar.allard@gmail.com>
@iameskild
Copy link
Member Author

Hi @trallard, thanks for the thorough review. Although I tried to incorporate as much of your feedback as possible, I wasn't able to get to all of it. You brought up a lot of good points that are definitely worth discussing with the rest of the team, namely tasks related to nebari-dev/nebari#1145. This will include these points you raised above:

Some additional thoughts:

  1. Right now the Dockerfiles are in . but there are directories for each image - this pattern is counter-intuitive I'd suggest moving the Docker images to the respective directories to keep everything aligned

  2. Additionally - and moving forward it be best to give more details on the infrastructure/ config of the repos this can be done either:

    1. Adding a Config files doc (.md) like the repo2docker one
    2. Adopting an INFRASTRUCTURE.md like the sphinx-book-theme one, pymc

Finally, I pinged @HarshCasper to help fill us in on how the Trivy vulnerability scan works but otherwise, I feel like this is ready for a final review :)

@iameskild
Copy link
Member Author

Hey @trallard, I believe this PR is ready to be merged, any final thoughts?

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Member

@trallard trallard left a comment

Choose a reason for hiding this comment

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

Looks good - added some trivial comments/ suggestions that I will incorporate sight away and merge 🚀

@trallard trallard merged commit 6156254 into nebari-dev:main Jul 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: dependencies 📦 needs: changes 🧱 Review completed - some changes are needed before merging
Projects
Development

Successfully merging this pull request may close these issues.

[ENH] - Moving docker images to separate repository
3 participants