Skip to content
This repository has been archived by the owner on Jun 22, 2024. It is now read-only.

Seleniarm add chromium arm64 circle #58

Open
wants to merge 12 commits into
base: trunk
Choose a base branch
from

Conversation

diemol
Copy link
Member

@diemol diemol commented Dec 19, 2023

Thanks for contributing to the Docker-Selenium project!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines, applied for this repository.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

Motivation and Context

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@diemol
Copy link
Member Author

diemol commented Dec 19, 2023

c.c. @fhoeben

@fhoeben
Copy link

fhoeben commented Dec 19, 2023

I see that forgot to set the correct namespace when I cherry picked the addition of the video workflow, I've created #59

@fhoeben
Copy link

fhoeben commented Dec 19, 2023

@diemol In your merge you forgot to remove NodeFirefox/Dockerfile.multi-arch that file is no longer used (and causes issue on vulnerability scan)

@diemol
Copy link
Member Author

diemol commented Dec 19, 2023

@jamesmortensen do you know why the push fails now?

@fhoeben
Copy link

fhoeben commented Dec 19, 2023

@jamesmortensen do you know why the push fails now?

Maybe the user does not have sufficient privileges to create new repositories in seleniarm?

For each image I also create a target architecture specific repository. I push the target's images there before combining them into a cross platform manifest in the normal repository. This to prevent a lot of extra tags in the normal repository, there you only want to see the combined manifest. But to create the manifest the images must first be pushed to the target registry server, so I opted to push to separate repositories. But (especially on first push) the user doing that must have rights to create the repo and later to push to it. I don't know, but maybe that is the issue?

Anyway the error occurs on the initial push of the generated image so the push is going to (for instance) seleniarm/base_arm64 which I suppose was not done before.

@jamesmortensen
Copy link

Is there a way to do this without creating the extra repositories? (I do keep the permissions of DOCKER_USER set to minimal to avoid any mistakes when pushing. The extra manifests may be why I avoided the manifest route in the past because it clutters up the namespace with non-multi-arch images.)

I changed to DOCKER_TEST_USERNAME and DOCKER_TEST_PASSWORD for now, which publishes to the "jamesmortensen1" namespace. If the push I just did works, you'll be able to try out the images and see if they work. (The tests may pass, but with such extensive changes to the build process, versioning, OS, etc, we'll want to make sure that everything works before publishing live.)

@fhoeben
Copy link

fhoeben commented Dec 19, 2023

@jamesmortensen

I started with just using other/extra tags in the main repositories, but that was really too much and too cluttering.
I like this approach in that each repository remains fairly clean, but indeed we get many more repositories.

In another project I use buildx explicitly (with Bake, which I think I prefer over Make), that offers an option to push its output to a repository directly but only based on the sha256 (so no tag is created). These shas are then captured and used to create the manifests. It keeps the repositories nice and clean, but does add complexity to the build process. (I fear the images we load into docker from cache will not be available in the build-engine used by buildx so we have to find a way around that. I found all that just getting too complex and error-prone.

Maybe we can have a separate namespace, or just a single repository which we use as a tmp/cache? Would that be a better approach?

@jamesmortensen
Copy link

@fhoeben the single cache repository sounds like a good idea. You can use "jamesmortensen1" namespace (DOCKER_TEST_USERNAME and DOCKER_TEST_PASSWORD as the temporary cache. Secrets are already setup for it.

Also, we should test the images to make sure they work, now that they're in Docker. I can look into that as well. I use https://github.com/jamesmortensen/debug-tools-for-docker-selenium to help run some basic manual tests on the images.

@fhoeben
Copy link

fhoeben commented Dec 19, 2023

@jamesmortensen I don't believe the secrets are accessible to me yet. At least when I created a PR it didn't seem to get the docker username. Or maybe I should not have done a PR, but ...?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants