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

Modified make build process to use --build-arg instead of generate.sh scripts, eliminating the templates. #1656

Merged
merged 5 commits into from
Aug 17, 2022

Conversation

jamesmortensen
Copy link
Member

@jamesmortensen jamesmortensen commented Aug 14, 2022

This PR takes advantage of the dynamic image feature where we pass in the namespace, version, authors, and in some instances, the base image to be used as the FROM argument, and fixes #1649

Description

  • Modified all Dockerfiles to use --build-arg to pass in namespace, version, authors, and in the case of standalone browser images, the base image.
  • Removed Dockerfile.txt files, which are no longer needed.
  • Removed generate.sh scripts, which are no longer needed.
  • Removed the "auto-generated" warnings from all of the Dockerfiles.
  • Removed the StandaloneChrome, StandaloneFirefox, StandaloneEdge folders as these images can all be built from the Standalone folder.
  • Modified Makefile: Removed all generate_x commands and added --build-args to the docker build commands via a common Makefile variable FROM_IMAGE_ARGS

Motivation and Context

This reduces the amount of files needed for the build process, and it simplifies the lines of code in the Makefile. It also eliminates commits to auto-generated files, which also eliminates unneeded conflicts when syncing forks.

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.

… scripts. Removed unneeded txt files and sh files.
@jamesmortensen
Copy link
Member Author

I see there's some conflicts. I'll work to resolve them as soon as I can.

… the Standalone browser container images. Resolved a warning caused by the Base and video Dockerfiles not needing the FROM_IMAGE_ARGS.
@jamesmortensen jamesmortensen requested a review from diemol August 14, 2022 20:39
@jamesmortensen
Copy link
Member Author

The tests in GitHub Actions now pass, and conflicts are resolved. Please let me know if there's anything else we should change. Otherwise, I feel this is good to go.

Since the changes were all internal to the Makefile, and since the arguments to the make build process are still the same, I don't think any changes to the README or docs are necessary. Please let me know if that's not the case.

@jamesmortensen
Copy link
Member Author

I want to add that changes to test.py were required. test.py rebuilds containers using the Docker SDK for Python, and this required the new build arguments passed into the SDK build method. This required the following additional changes:

  • Created a FROM_IMAGE_ARGS mapping with the NAMESPACE and VERSION for the Dockerfile FROM argument. This dictionary is passed into the Docker SDK build method as buildargs.
  • Since we eliminate the Standalone[Chrome|Edge|Firefox) folders, we have a get_build_path method to use "Standalone" as the build folder for the browser images and use the original container mapping key for all others.
  • Created set_from_image_base_for_standalone method to get the correct base image to use for each standalone browser image. Additional browsers are easily added by modifying the standalone_browser_container_matches regular expression.

My goal was to make it as easy as possible to maintain. For instance, if we ever add Opera back, or WebKit, then the regular expression would be changed from (Standalone)(Chrome|Firefox|Edge) to (Standalone)(Chrome|Firefox|Edge|Opera|WebKit).

@jamesmortensen jamesmortensen marked this pull request as draft August 16, 2022 05:20
@diemol
Copy link
Member

diemol commented Aug 16, 2022

Any reason why this is now a draft? Looked good to me.

@jamesmortensen
Copy link
Member Author

@diemol I wanted to validate that none of my changes affected the deploy process. I haven't done that yet, but if you think it is good to go, then we can merge.

@jamesmortensen jamesmortensen marked this pull request as ready for review August 17, 2022 01:16
@jamesmortensen
Copy link
Member Author

Test deployment looks good. I think we're ready to merge: https://github.com/jamesmortensen/docker-selenium/releases/tag/4.4.0-20220817

Copy link
Member

@diemol diemol 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, @jamesmortensen! This is great!

@diemol diemol merged commit 7010a13 into SeleniumHQ:trunk Aug 17, 2022
jamesmortensen added a commit to seleniumhq-community/docker-seleniarm that referenced this pull request Aug 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants