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

[HPR-1261] Add docker full image #12532

Merged
merged 7 commits into from
Aug 4, 2023
Merged

Conversation

lbajolet-hashicorp
Copy link
Contributor

Part of the bundled plugin removal effort, this PR adds a new Docker target: release-full.
This image includes Packer, and all the official plugins pre-installed in its environment.

@lbajolet-hashicorp lbajolet-hashicorp requested a review from a team July 21, 2023 16:21
@lbajolet-hashicorp lbajolet-hashicorp requested a review from a team as a code owner July 21, 2023 16:21
Copy link
Contributor

@nywilken nywilken left a comment

Choose a reason for hiding this comment

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

This looks good to me I added a commit to add the docs for the full version e25a24f. Please take a look over them and adjust as you see fit.

I know the docker build action handles the building of the containers so it is not needed but I do wonder if we should update the Makefile for the Packer project to include a target for the release-full target?

Maybe we should at least bump the version of Packer within the Makefile if we don't choose to add a target for the release-full

@nywilken nywilken added backport/1.9.x Backport PR changes to `release/1.9.x` enhancement labels Jul 24, 2023
Copy link
Contributor

@nywilken nywilken left a comment

Choose a reason for hiding this comment

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

A couple of questions but looking good.

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@lbajolet-hashicorp lbajolet-hashicorp force-pushed the add_docker_full_image branch 2 times, most recently from 9755e54 to 3864d8d Compare August 1, 2023 21:21
@nywilken nywilken changed the title Add docker full image [HPR-1261] Add docker full image Aug 2, 2023
Copy link
Contributor

@nywilken nywilken left a comment

Choose a reason for hiding this comment

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

The changes to the build pipeline, Dockerfile, and Makefile look good to me. But the Docker README which gets published to DockerHub needs to be updated.

Can you please update to reflect the changes to the product Docker images.

@lbajolet-hashicorp
Copy link
Contributor Author

Good call, I didn't know about this one, I'll update it this morning

Since the main branch is not called master anymore, and the anchor was
renamed, we fix both these changes in the link to that page from the
README.
In addition to the `release-light' target, we add a `release_full'
target to the Dockerfile, so that we can ship an image of Packer with
the official plugins pre-bundled in their latest version.
The Docker images release-light and release-full are not to be built
locally from a dev build, but from a release, and the commands to build
those images are only referenced in CI, so we don't need to ship them as
part of the makefile.
In addition, those images are not straightforward to build from the
Makefile, as they require quite a few things from the environment, as
well as the binary installed in a specific location, which is never
setup by the rest of the Makefile.

Therefore, we opted to simplify the Makefile so that it only builds
docker-dev for local use.
@lbajolet-hashicorp lbajolet-hashicorp force-pushed the add_docker_full_image branch 2 times, most recently from 1679998 to 636215c Compare August 3, 2023 14:15
.release/docker/README.md Outdated Show resolved Hide resolved
.release/docker/README.md Outdated Show resolved Hide resolved
.release/docker/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@nywilken nywilken left a comment

Choose a reason for hiding this comment

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

I left a few suggestions but this otherwise looks good.

@lbajolet-hashicorp
Copy link
Contributor Author

Perfect thanks, I just integrated your suggestions, unless you have an objection, I say we merge this when tests are green, sounds good to you @nywilken ?

@nywilken
Copy link
Contributor

nywilken commented Aug 4, 2023

Perfect thanks, I just integrated your suggestions, unless you have an objection, I say we merge this when tests are green, sounds good to you @nywilken ?

:shipit:

@github-actions
Copy link

github-actions bot commented Sep 4, 2023

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/1.9.x Backport PR changes to `release/1.9.x` enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants